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 config for formatting and linting #1166

Merged
merged 1 commit into from
Apr 26, 2020

Conversation

ejochman
Copy link
Contributor

Adds a pre-commit config that runs several fixers, including YAPF with
"google" style and pylint for static analysis

Note: This is a local pre-commit only. Running a GitHub action to perform this action on PRs may be configurable.

Makes progress on #874 to define a style guide. May still need to setup a GitHub action to enforce on PRs.

@jay0lee
Copy link
Member

jay0lee commented Apr 22, 2020

requirements.txt should only be what's necessary to build GAM, not develop. We may need some kind of dev requirements....

Adds a pre-commit config for development that runs several fixers, including YAPF with
"google" style and pylint for static analysis.

Use of `requirements-dev.txt` appears to be a common pattern for this
type of work:
https://pypi.org/project/requirements-dev.txt/
@ejochman
Copy link
Contributor Author

I moved the pre-commitrequirement to requirements-dev.txt which appears to be a commonly utilized pattern, per https://pypi.org/project/requirements-dev.txt/

@ejochman ejochman marked this pull request as ready for review April 23, 2020 18:33
@jay0lee
Copy link
Member

jay0lee commented Apr 24, 2020

Thanks @ejochman

If I understand correctly we won't need Travis to install requirements-dev.txt but GAM developers should? What happens if pre-commit isn't installed? Will git fail or will it just allow the commit without running pre-commit?

@ejochman
Copy link
Contributor Author

I think developers would have to install requirements-dev.txt and then run pre-commit install to install pre-commit in their git hooks. From there, it should run before every git commit. It looks like there's a github action we could use to run it, but it doesn't seem to scope the pre-commit results to only the files modified in the commit, out of the box (there may be a way to do this, but I haven't read into the Actions api enough to figure it out). Running pre-commit on all files is great for the long-term where everything in the repo is clean and only new changes would introduce issues. However, right now, it's a bit of a mess, and each PR is going to have a laundry list of issues until we do a massive cleanup. See this example. Contrast that with the local pre-commit where it will only run the linter/formatter against files that are in the commit, which is a bit easier to handle in most cases.

@ejochman
Copy link
Contributor Author

Sorry. To more specifically answer your question:

Will git fail or will it just allow the commit without running pre-commit?

If the dev doesn't have pre-commit installed, nothing happens and the commit succeeds as it does today. If it is installed, then it will prevent them from committing until the issues are fixed and staged for commit, or they bypass the pre-commit with a flag.

My suggestion: We introduce this pre-commit config locally, first (just to get us all on a baseline config that we can work against). I can put some effort into auto-cleaning most of the codebase and followup with some easy linter fixes. Then, we can see where we're at and how much further we'd have to get, in order please all the linter/formatter errors and decide if it's the right time to add the github action. Thoughts?

@jay0lee jay0lee merged commit f301dac into GAM-team:master Apr 26, 2020
@ejochman ejochman deleted the pre-commit branch April 28, 2020 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants