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

Linting, CI, dependencies, and infrastructure updates #810

Merged
merged 11 commits into from
Jul 31, 2023

Conversation

rkingsbury
Copy link
Collaborator

@rkingsbury rkingsbury commented Jun 27, 2023

This PR contains a series of updates to the linting, CI, and dependency management systems to make package maintenance easier and to harmonize our practices with other MP ecosystem packages, notably pymatgen and custodian.

Specifically:

  • replace flake8, pycodestyle, pydocstyle, and isort dependencies with ruff (note that pydocstyle was in the requirements but never invoked in CI or pre-commit). Ruff is substantially faster than flake8, etc., and replaces many popular tools by re-implementing rules from pycodestyle, pydocstyle, flake8, isort, etc. into a single dependency. The ruff configuration closely tracks that implemented by @materialsproject/pymatgen-maintainers for custodian and pymatgen (see Add ruff linter custodian#278)
  • add ruff linting to CI (replacing Flake8)
  • replace setup.cfg with pyproject.toml
  • add GitHub workflow to periodically update dependencies in a single PR (alternative to dependabot). See https://www.oddbird.net/2022/06/01/dependabot-single-pull-request/
  • lint whole project with ruff, which required moving some mypy imports from inline comments (e.g., # type: Any) to normal type hints and making use of teh __all__ variable in empty __init__.py files.
  • update .pre-commit action to current repo and version
  • add pre-commit auto fixes to CI
  • updated PR template to sync more closely with pymatgen
  • misc. spelling fixes

@codecov
Copy link

codecov bot commented Jun 27, 2023

Codecov Report

Patch coverage: 87.30% and project coverage change: +0.06% 🎉

Comparison is base (ad355cf) 88.09% compared to head (cbaefa6) 88.15%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #810      +/-   ##
==========================================
+ Coverage   88.09%   88.15%   +0.06%     
==========================================
  Files          44       44              
  Lines        3629     3589      -40     
==========================================
- Hits         3197     3164      -33     
+ Misses        432      425       -7     
Files Changed Coverage Δ
src/maggma/__init__.py 100.00% <ø> (ø)
src/maggma/api/API.py 81.81% <ø> (ø)
src/maggma/api/query_operator/core.py 100.00% <ø> (ø)
src/maggma/api/query_operator/pagination.py 100.00% <ø> (+4.00%) ⬆️
src/maggma/api/resource/core.py 83.72% <ø> (+3.72%) ⬆️
src/maggma/api/resource/s3_url.py 58.33% <0.00%> (ø)
src/maggma/api/resource/utils.py 80.76% <0.00%> (ø)
src/maggma/core/validator.py 100.00% <ø> (ø)
src/maggma/utils.py 94.01% <60.00%> (-1.71%) ⬇️
src/maggma/cli/distributed.py 81.09% <63.63%> (ø)
... and 28 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rkingsbury rkingsbury marked this pull request as ready for review June 27, 2023 21:33
@rkingsbury rkingsbury added release:minor dependencies Pull requests that update a dependency file github_actions Pull requests that update Github_actions code and removed release:minor labels Jun 27, 2023
@rkingsbury rkingsbury changed the title switch linting to ruff Linting, CI, dependencies, and infrastructure updates Jul 31, 2023
@rkingsbury rkingsbury requested a review from munrojm July 31, 2023 14:15
@janosh
Copy link
Member

janosh commented Jul 31, 2023

Looks great! Thanks @rkingsbury.

and making use of teh all variable in empty init.py files.

I'm curious what you mean by that. Is this about unused import errors?

Yes, initially when running ruff it did not like using __init__.py to collect imports, so I placed all imported classes inside the __all__ variable. This was before I duplicated your ruleset; I think your per-file-ignore "__init__.py" = ["F401"] takes care of it.

src/maggma/stores/mongolike.py Dismissed Show dismissed Hide dismissed
src/maggma/stores/mongolike.py Dismissed Show dismissed Hide dismissed
pyproject.toml Outdated
Comment on lines 83 to 86
"src/maggma/api/*" = ["B008", "B021", "RET505", "RET506"]
"tests/api/*" = ["B017", "B018"]
"src/maggma/cli/*" = ["EXE001"] # triggered by ! at top of file
"src/maggma/api/utils.py" = ["I001"] # to allow unsorted import block
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@munrojm I disabled a few ruff rules in api because I didn't want to inadvertently screw anything up. You may want to examine and see if any can be fixed

@rkingsbury
Copy link
Collaborator Author

@munrojm any concerns about the migration to pyproject.toml, or other changes here? I'll wait to hear from you before merging since this touches a lot of the CI infrastructure.

@munrojm
Copy link
Member

munrojm commented Jul 31, 2023

Nope! No concerns on my end. I'm glad to have these changes. Thanks for making them.

@rkingsbury rkingsbury merged commit 009a4f8 into materialsproject:main Jul 31, 2023
10 of 11 checks passed
@rkingsbury rkingsbury deleted the rufflint branch July 31, 2023 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file github_actions Pull requests that update Github_actions code release:minor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants