Move ruff, coverage, and pytest configs to pyproject.toml #3557
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
Missed this when I reviewed, but I think the errors are because "--rcfile=.coveragerc" needs to be removed from the noxfile for coverage |
|
Also, the tests for the example notebooks are failing too. I think the |
pyproject.toml
Outdated
| @@ -174,6 +174,8 @@ pybamm = [ | |||
|
|
|||
| [tool.setuptools.packages.find] | |||
| include = ["pybamm", "pybamm.*"] | |||
| # TODO: remove once https://github.com/pybamm-team/PyBaMM/issues/3480 is resolved | |||
There was a problem hiding this comment.
Unrelated, but do we have and process for periodically checking TODOs to make sure we are taking care of them over time?
There was a problem hiding this comment.
It should be a good idea to add a workflow to do that. I don't remember the name exactly, but there is a pre-commit hook too that can check for # TODO: and # FIXME: comments.
There was a problem hiding this comment.
Also, this # TODO: comment in specific should not be added imo, I remember that when I worked on this part of the configuration it was also trying to install the tests, examples, and the benchmarks folders treating them as packages, because they are not namespace packages. Including just pybamm and the modules under it is the most reasonable solution; we should look to configure this from pytest instead.
pyproject.toml
Outdated
| [tool.pytest] | ||
| # Use pytest-xdist to run tests in parallel by default, exit with | ||
| # error if not installed | ||
| required_plugins = [ | ||
| "pytest-xdist", | ||
| ] | ||
| addopts = [ | ||
| "-nauto", | ||
| "-v", | ||
| ] | ||
| testpaths = [ | ||
| "docs/source/examples/", | ||
| ] | ||
| console_output_style = "progress" | ||
|
|
There was a problem hiding this comment.
I think the norecursedirs configuration value should do it: https://docs.pytest.org/en/7.1.x/reference/reference.html#confval-norecursedirs
There was a problem hiding this comment.
This surprisingly did not work (it should have according to the docs). Ignoring pybind11 in addopts works for me locally.
There was a problem hiding this comment.
I think it failed again on the latest commit with addopts, up to you if you want to try another option or wait for #3480 to be completed – I am almost done!
There was a problem hiding this comment.
Turns out, this wasn't a pytest+pybind11 issue. I was specifying pytest's configuration under [tool.pytest], but it should have been [tool.pytest.ini_options].
Co-authored-by: Eric G. Kratz <kratman@users.noreply.github.com>
…nto ruff-config
| # "B", # flake8-bugbear | ||
| # "I", # isort | ||
| # "ARG", # flake8-unused-arguments | ||
| # "C4", # flake8-comprehensions | ||
| # "ICN", # flake8-import-conventions | ||
| # "ISC", # flake8-implicit-str-concat | ||
| # "PGH", # pygrep-hooks | ||
| # "PIE", # flake8-pie | ||
| # "PL", # pylint | ||
| # "PT", # flake8-pytest-style | ||
| # "PTH", # flake8-use-pathlib | ||
| # "RET", # flake8-return | ||
| "RUF", # Ruff-specific | ||
| # "SIM", # flake8-simplify | ||
| # "T20", # flake8-print | ||
| # "UP", # pyupgrade | ||
| "YTT", # flake8-2020 |
There was a problem hiding this comment.
I do want to add all these rules but this PR will blow up with formatting changes if I enable all of them at once.
There was a problem hiding this comment.
We can always just review the config stuff then enable the other checks right before a final review. I do that sometimes on other projects.
There was a problem hiding this comment.
Should we leave them for a follow-up PR?
We can always just review the config stuff then enable the other checks right before a final review. I do that sometimes on other projects.
That would work too
There was a problem hiding this comment.
I was figuring that we should do it all at once since we suppress these changes in git blame.
There was a problem hiding this comment.
I think follow-up PRs would be best here. That would also ensure that we have everyone's consensus on adding thee individual rule. For instance, we used to have isort in pybamm somewhere in the past but then it was removed on everyone's consensus. I'll open an issue for this once the PR is merged.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3557 +/- ##
========================================
Coverage 99.58% 99.58%
========================================
Files 256 256
Lines 20126 20131 +5
========================================
+ Hits 20043 20048 +5
Misses 83 83 ☔ View full report in Codecov by Sentry. |
Description
I've left cibuildwheel as of now as keeping its config in the CI files seems more accessible.
Fixes #3478
Type of change
Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #) - note reverse order of PR #s. If necessary, also add to the list of breaking changes.
Key checklist:
$ pre-commit run(or$ nox -s pre-commit) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)$ python run-tests.py --all(or$ nox -s tests)$ python run-tests.py --doctest(or$ nox -s doctests)You can run integration tests, unit tests, and doctests together at once, using
$ python run-tests.py --quick(or$ nox -s quick).Further checks: