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

use pre-commit to invoke linters #1591

Merged
merged 28 commits into from
Jul 10, 2023
Merged

use pre-commit to invoke linters #1591

merged 28 commits into from
Jul 10, 2023

Conversation

williballenthin
Copy link
Collaborator

@williballenthin williballenthin commented Jul 7, 2023

adds a pre-commit configuration so that its easy to invoke all of our linter checks w/associated configuration, as well as install the pre-commit and pre-push hooks consistently:

image

quickstart

do this now @mr-tz @mike-hunhoff @yelhamer @colton-gabertan (FYI)

$ pre-commit install    # now pre-commit hook installed!

or if you want to run all the linters on-demand:

$ pre-commit run --all-files

the PR also introduces some additional plugins (and fixes) for flake8:

  • bugbear - find likely bugs, which it did, such as use of str.lstrip("many characters here")
  • encodings - enforce text read from files has an explicit encoding
  • comprehensions - use list/tuple/set comprehensions where appropriate (and don't, when not appropriate)
  • logging-format - don't use format strings in logging calls
  • no-implicit-concat - catch potential bugs by disallowing implicit string concatenation, like "foo" "bar" == "foobar"
  • print - catch print statements in library code
  • todos - enforce TODOs are formatted like TODO(author): message and have an associated link to a tracking github issue

im really excited about some of these lints (like logging format), since we've often had to explain and manually check for these during PR reviews. the other ones enforce better hygiene (imho).

closes #1579
closes #1391

Checklist

  • No CHANGELOG update needed
  • No new tests needed
  • documentation update

@williballenthin williballenthin added documentation Improvements or additions to documentation enhancement New feature or request CI Continuous Integration configuration dependencies Pull requests that update a dependency file labels Jul 7, 2023
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Please add bug fixes, new features, breaking changes and anything else you think is worthwhile mentioning to the master (unreleased) section of CHANGELOG.md. If no CHANGELOG update is needed add the following to the PR description: [x] No CHANGELOG update needed

@github-actions github-actions bot dismissed their stale review July 7, 2023 08:48

CHANGELOG updated or no update needed, thanks! 😄

Copy link
Collaborator

@mr-tz mr-tz left a comment

Choose a reason for hiding this comment

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

looks solid!

@williballenthin
Copy link
Collaborator Author

just getting tests to pass and then i'll open this up for review

@williballenthin williballenthin marked this pull request as ready for review July 7, 2023 10:35
doc/installation.md Outdated Show resolved Hide resolved
mr-tz

This comment was marked as resolved.

@williballenthin
Copy link
Collaborator Author

i have a few more linters in the works (flake8 plugins) so i'll get those hammered out before we merge.

@williballenthin williballenthin marked this pull request as draft July 7, 2023 11:41
@williballenthin williballenthin marked this pull request as ready for review July 10, 2023 09:27
@williballenthin williballenthin merged commit 506d677 into master Jul 10, 2023
7 checks passed
@williballenthin williballenthin deleted the fix/issue-1579 branch July 10, 2023 09:58
@mr-tz
Copy link
Collaborator

mr-tz commented Jul 10, 2023

Wow, nice improvements. Thanks!

@williballenthin
Copy link
Collaborator Author

williballenthin commented Jul 14, 2023

@ajnelson-nist check out how we integrated pre-commit here (thanks to your pointer).

specifically, we use a local configuration rather than using the third-party pre-commit hooks. this lets us declare the linter dependencies in setup.py/pyproject.toml and therefore get updated by Dependabot while still getting invoked by pre-commit.

https://github.com/mandiant/capa/blob/f983307c978b82911d823ac291f4e397393f4ba2/.pre-commit-config.yaml

@ajnelson-nist
Copy link

@williballenthin And now you've explained something I hadn't quite understood from the examples I'd drawn from. Local configurations! That's how they'd gotten around pinning-vs-range version freshness issues.

This was a fruitful back and forth. Thanks for letting me know what you found!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous Integration configuration dependencies Pull requests that update a dependency file documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

migrate code style checks to pre-commit triage existing TODOs and schedule re-reviews
3 participants