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 and apply all fixes #2139

Merged
merged 1 commit into from
Oct 27, 2022
Merged

Conversation

hackedd
Copy link
Collaborator

@hackedd hackedd commented Oct 25, 2022

This PR installs pre-commit in the Pipenv environment, and updates the make style command to use it (as wel as running before commits, of course).

I've configured hooks for the tools that were already used (Flake8, Pylint, Black) and created a new hook for gsemet/fiximports. This last hook is pinned to a commit hash in my fork for now, but can be updated to a tag once the PR over there is merged.

Apart from these hooks, I've enabled end-of-file-fixer (which ensures files end with a newline) and trailing-whitespace. Most of the changes are from the initial pre-commit run --all-files.

Closes #2031.

@hackedd hackedd force-pushed the pre-commit branch 2 times, most recently from c806542 to 3f65c3a Compare October 25, 2022 15:32
@hackedd
Copy link
Collaborator Author

hackedd commented Oct 25, 2022

I've pinned pre-commit to version 2.17.0, newer releases no longer support Python 3.6 (which reached end of life December of last year).

Copy link
Collaborator

@Davidy22 Davidy22 left a comment

Choose a reason for hiding this comment

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

Seems mostly fine, nice to get the whitespace cleaner into the chain, just a thing about the black version pinning

Pipfile Outdated Show resolved Hide resolved
Copy link
Collaborator

@Davidy22 Davidy22 left a comment

Choose a reason for hiding this comment

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

At some point we can probably drop support for python 3.6 which'll free up deps that depend on it, and I'm thinking we can bump black when it rolls over to the 2023 standard, but that's something to consider for the future when 3.6 shrinks some more and we actually get to see a 2023 black release. For now, this looks fine, merging.

@Davidy22 Davidy22 merged commit 86bb9b5 into Guake:master Oct 27, 2022
@Davidy22
Copy link
Collaborator

I've noticed a fair few pretty good PRs from you, I wouldn't mind giving you reviewer privileges if you want them. They're far from an obligation to do reviewing, as the long list of inactive people with review rights would attest, but I certainly appreciate seeing more active hands in the project. I've been a bit busy at times myself, changes out of me have been a bit spread apart for a while due to regular work and getting discouraged at trying and failing to reproduce issues so sometimes it takes a while for me to get around to looking at PRs or new issues.

@hackedd hackedd deleted the pre-commit branch October 27, 2022 07:44
@hackedd
Copy link
Collaborator Author

hackedd commented Oct 27, 2022

Thanks, I appreciate it. I wouldn't mind being a bit more involved in this project, as I've happily used Guake for years :)

@Davidy22
Copy link
Collaborator

Alright, invite sent

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.

use pre-commit in the git hook check
2 participants