PyBaMM
PyBaMM copied to clipboard
pybamm-team
Review ruff rules
I think we never looked at the ruff rules after it was added to the codebase. Some of these are very useful and should be selected.
https://github.com/pybamm-team/PyBaMM/blob/10a404eb2f90d263b3bc78e1704f9e881b1d9923/pyproject.toml#L172-L212
For someone looking at this issue: feel free to pick up a rule and discuss why it should be included/excluded in this thread before making a PR.
I do feel I (isort), ICN can be included to standardize imports, PT since we use pytest for our testing suite and recently had the addition of fixtures as well (PT020, PT001, PT003, PT014) and C4 to get rid of unnecessary generators and list comprehensions maybe?
I ran ruff check --fix . for the isort rules, and it changed nearly 400+ files. Should I include all in a single pr ? and for the Pytest Rule it suggests decent amount of changes to be done, I guess seperate PRs for each should work?
I'd say let's go ahead with the pytest rule and discuss isort more in this thread.
I hope the pytest rule is not asking us to remove classes?
I think including all the isort fixes in the same PR would be fine. Even if enabling the rule changes 400+ files, most of the changes would be at the top of the files anyway, which are less likely to cause merge conflicts with other PRs. One important thing would be to ensure that imports within functions remain unchanged. We use such imports in many places as a part of optional dependencies we ship. They can be marked with inline # noqa: <corresponding rule ID> comments so that they are not processed.
The errors from pytest rule are mostly around pytest.raises() being too broad without a match parameter (PT011), the pytest.raises block containing multiple statements under its context (PT012) - should be limited to a single statement
and pytest.mark.parameterize must have a tuple for the initial parameters if it has a sequence of strings (PT006)