Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add pre-commit command #58

Merged
merged 44 commits into from
Mar 23, 2022
Merged

Conversation

blink1073
Copy link
Collaborator

@blink1073 blink1073 commented Mar 18, 2022

  • Refactor blackify into utility functions for prepping the repo and pushing work
  • Add a "pre-commit" command target that runs pre-commit as a new commit and pushes.
  • Test black and pre-commit in a PR
  • Add docstrings
  • Add documentation
  • Make a run convenience function that prints the command
  • Make keen optional so it isn't needed in non-prod apps (costs $300/mo on team account)
  • Add contributor guide
  • Add pre-commit config with flake8
  • Add GitHub Actions testing (will require a B64KEY secret in this repo) - passing test

This is an on-demand alternative to the more disruptive "auto fixing pull requests" from https://pre-commit.ci/

@blink1073
Copy link
Collaborator Author

cc @choldgraf @consideRatio

Copy link
Contributor

@Carreau Carreau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know if you have access to heroku to debug if there are issues.

@blink1073
Copy link
Collaborator Author

On further thought, this won't work because if the hook changes anything it is considered a failure. Will need to do some more refactoring.

@blink1073 blink1073 marked this pull request as draft March 18, 2022 10:45
@blink1073 blink1073 marked this pull request as ready for review March 18, 2022 11:11
@blink1073
Copy link
Collaborator Author

@Carreau I don't think I have access to that heroku app, my account is under steven.silvester at gmail.com.

@blink1073
Copy link
Collaborator Author

Oh I see what you mean, I can deploy this myself.

@consideRatio
Copy link

Hi @blink1073!
Let me know if you want review or input with something specific @blink1073!

Note that I've not worked on this bot and hasn't used it besides learning that it can be used for backporting PRs by Min. Quickly glancing at the code I didn't see docstrings which otherwise could help me review something unknown by just knowing that the docstring declares the objective of a function.

I guess a technical review point could be that it would be good if you add docstrings to any function you add or re-purpose to help newcomers! :)

@blink1073 blink1073 marked this pull request as draft March 18, 2022 16:28
@blink1073
Copy link
Collaborator Author

Converting to draft while I test in blink1073#1. I'm putting this aside for the weekend.

@Carreau
Copy link
Contributor

Carreau commented Mar 18, 2022

To deploy you can just merge this.
The heroku access is mostly for heroku logs --tail to help debugging if it fails somewhere.

@Carreau
Copy link
Contributor

Carreau commented Mar 18, 2022

(and feel free to merge-try-revert to test if you need.)

@blink1073 blink1073 marked this pull request as ready for review March 23, 2022 10:50
@blink1073
Copy link
Collaborator Author

@Carreau I've finished. I ended up adding flake8 and GitHub Actions support.

@blink1073
Copy link
Collaborator Author

@consideRatio thanks for the feedback! I was pinging you and Chris as potential users of the new feature, no review required.

@Carreau
Copy link
Contributor

Carreau commented Mar 23, 2022

Ok, let's just merge and see if things break. <3

@Carreau Carreau merged commit 7f2f8cf into scientific-python:master Mar 23, 2022
@blink1073
Copy link
Collaborator Author

@Carreau can you please add me as a collaborator on the repo? I can add the env secret needed for the test

@Carreau
Copy link
Contributor

Carreau commented Mar 23, 2022 via email

@Carreau
Copy link
Contributor

Carreau commented Mar 23, 2022

Sorry, I thought you were.

I also invited you to the org in case you need even more permissions.

@blink1073 blink1073 deleted the pre-commit branch March 23, 2022 16:27
@blink1073
Copy link
Collaborator Author

I also invited you to the org in case you need even more permissions.

Cheers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants