Review ruff rules

[Saransh-cpp]

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.

PyBaMM/pyproject.toml

Lines 172 to 212 in 10a404e

	[tool.ruff.lint]
	extend-select = [
	"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
	"TID252", # relative-imports
	"S101", # to identify use of assert statement
	"PT027", # remove unittest style assertion
	"PT009", # Use pytest.raises instead of unittest-style
	]
	ignore = [
	"E741", # Ambiguous variable name
	"RUF012", # Mutable class attributes should be annotated with `typing.ClassVar`
	"SIM108", # Use ternary operator
	"ARG001", # Unused function argument:
	"ARG002", # Unused method arguments
	"PLR2004", # Magic value used in comparison
	"PLR0915", # Too many statements
	"PLR0913", # Too many arguments
	"PLR0912", # Too many branches
	"RET504", # Unnecessary assignment
	"RET505", # Unnecessary `else`
	"RET506", # Unnecessary `elif`
	"B018", # Found useless expression
	"RUF002", # Docstring contains ambiguous
	"UP007", # For pyupgrade
	]

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.

[Aswinr24]

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?

[Saransh-cpp]

Yes! PT and I definitely ✅

Should be added in separate PRs tho.

[Aswinr24]

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?

[Saransh-cpp]

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?

[agriyakhetarpal]

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.

[Aswinr24]

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)

[agriyakhetarpal]

Thanks, @Aswinr24. All three of those would be pretty nice to fix!

[Aswinr24]

Yes will work on fixing them!