Skip to content
This repository has been archived by the owner on Jul 3, 2023. It is now read-only.

[ci] enforce flake8 (fixes #161) #222

Merged
merged 1 commit into from
Oct 29, 2022
Merged

[ci] enforce flake8 (fixes #161) #222

merged 1 commit into from
Oct 29, 2022

Conversation

jameslamb
Copy link
Contributor

Proposes enforcing flake8's checks on for pull requests to be merged.

In my experience, flake8 is very helpful in detecting correctness and efficiency related issues such as:

  • unused imports
  • unused variables
  • duplicated (and therefore ignored) unit tests functions
  • unnecessary use of f-strings

And many more.

Changes

"resolves" the following flake8 warnings by annotating them with #noqa comments.

./hamilton/graph.py:113:6: F821 undefined name 'graphviz'
./hamilton/graph.py:139:6: F821 undefined name 'networkx'
./hamilton/graph.py:275:13: F401 'graphviz' imported but unused
./hamilton/data_quality/default_validators.py:415:9: F401 'pandera' imported but unused
./tests/resources/bad_functions.py:5:1: F401 'tests.resources.only_import_me' imported but unused
./tests/resources/cyclic_functions.py:5:1: F401 'tests.resources.only_import_me' imported but unused

Adds the latest version of flake8 as a new pre-commit task, ensuring that it's run in CI.

Adds a root-level setup.cfg containing configuration for flake8, including excluding some style-only warnings that are currently raised running flake8 . on main.

Testing

pre-commit run --all-files

Notes

Why setup.cfg instead of just adding configuration in .pre-commit-config.yaml?

Putting configuration for tools like flake8 into the standard files used in Python projects (e.g. setup.cfg or pyproject.toml) decouples the configuration from the thing running the tool. e.g. if this project were to switch from pre-commit run to a Makefile or shell script for running these checks, none of the flake8 configuration would need to be moved.

If maintainers feel strongly about not adding this file, I'd be happy to move this configuration back to .pre-commit-config.yaml.

Thanks for your time and consideration!

Checklist

  • PR has an informative and human-readable title (this will be pulled into the release notes)
  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code passed the pre-commit check & code is left cleaner/nicer than when first encountered.
  • Passes all existing automated tests
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future TODOs are captured in comments
  • Project documentation has been updated if adding/changing functionality.
  • Reviewers requested with the Reviewers tool ➡️

Testing checklist

Python - local testing

  • python 3.6
  • python 3.7

@@ -26,3 +26,7 @@ repos:
hooks:
- id: isort
args: ["--profile", "black", --line-length=100]
- repo: https://github.com/pycqa/flake8
rev: 5.0.4
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is the latest version as of today: https://pypi.org/project/flake8/#history

Copy link
Collaborator

@elijahbenizzy elijahbenizzy left a comment

Choose a reason for hiding this comment

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

LGTM!

@elijahbenizzy
Copy link
Collaborator

Happy with this, merging!

@elijahbenizzy elijahbenizzy merged commit f21ede1 into stitchfix:main Oct 29, 2022
@jameslamb
Copy link
Contributor Author

thanks @elijahbenizzy ! I think it'll be useful 😊

@jameslamb jameslamb deleted the add-flake8 branch October 29, 2022 17:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants