Upgrade and broaden flake8, fixing style problems and bugs by EliahKagan · Pull Request #1673 · gitpython-developers/GitPython (original) (raw)

Fixes #1670
Fixes #1671

I've upgraded flake8 in the pre-commit configuration, which automatically upgrades its pycodestyle dependency. This makes it compatible with Python 3.12 (or at least we seem unaffected by any remaining incompatibilities). I've also upgraded its non-default plugins, which are listed there as additional dependencies. I have fixed a warning that was for some reason specific to 3.12 about spacing around +, as well as some new warnings that came in the new version of pycodestyle and new versions of the non-default packages. This included changing == to is and != to is not when comparing type objects, since in all cases it appeared exact type matching was intended (rather than what an issubclass or, under simplification of the surrounding code, isinstance would check). See #1670.

I've also broadened flake8 to check the whole project (except the doc/ directory). That is, it now checks the test suite. It issued a number of warnings, and I fixed the code accordingly. Most of these are style changes, but it revealed #1671, which I fixed as described there (by using unittest.mock.patch.dict, which as noted there is okay because it is only in the tests). I then looked for bugs in finally cleanup logic throughout the project, in case anything else like that was present that I could identify and fix. I found no further serious bugs inside test/ while doing this, but I did find some areas I could simplify or otherwise improve, including one place where a throwaway environment variable FOO was never unpatched.

I included those changes in this PR, but not any changes to code in git/ other than to fix what flake8 found. One place in git/ that should be changed is a bug that merits fixing in some way, which I opened #1669 for. Other areas of improvement in git/ related to the use of finally are less important, and some of them subjective. I could include them in a fix for #1669 (if I end up fixing it) or in a separate PR, but I've omitted those further changes in git/ from this PR to keep its scope from creeping too large.

An area that I think deserves special attention in review is changes I made in parts of the test code that are reflected in built documentation that will be published on readthedocs:

• Where @NoEffect appeared in code that is copied to automatically generate usage examples in the documentation, I expanded it to # noqa: B015 # @NoEffect. Readability seems still intact, and I am inclined to think this is justified as long as we need to have @NoEffect. However, if @NoEffect is no longer needed, then I think configuring flake8 to disable the specific warning in those two specific test modules would be better, since then neither @NoEffect nor # noqa: B015 would need to appear anywhere in documentation example code.
• Comprehensions that perform neither mapping nor filtering are only occasionally useful, and almost never when they are list comprehensions: code like [c for c in commits_for_file_generator] should be list(commits_for_file_generator) instead. Expressions of that form appear a number of times in tests from which documentation is automatically generated, and I think changing it in this way is only beneficial. However, since it is a code change that will affect documentation (which might otherwise be unexpected), I do want to call attention to it, too.

In most cases I have retained existing noqa comments, and I suspect a number of them could be removed. That could be done at any time, and it was also only to limit scope that I have not tried to do it here. Besides that, however, there is a different reason I have deliberately avoided going further with flake8--for example, by adding it and its extra plugins as development dependencies of the project (they are still only installed by and for pre-commit), running it on CI, enabling more checks, or attempting to get the flake8-type-checking plugin listed in requirements-dev.txt working. The reason is that I think it would be beneficial to replace flake8 in this project with ruff, which is modern, versatile, and extremely fast. (This is also why I have not proposed that we also adopt isort, even though isort is one of my favorite tools: ruff might take care of that, too.)