GeezerVison is showing github.com — Upgrade and broaden flake8, fixing style problems and bugs by EliahKagan · Pull Request #1673 · gitpython-developers/GitPython
Conversation
EliahKagan
commented
Sep 21, 2023
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
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.)
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Upgrading flake8 from 6.0.0 to 6.1.0 causes its pycodestyle
dependency to be upgraded from 2.10.* to 2.11.*, which is desirable
because:
- Spurious "E231 missing whitespace after ':'" warnings on 3.12 due
to the lack of full compatibility with Python 3.12 are gone.
- New warnings appear, at least one of which, "E721 do not compare
types, for exact checks use `is` / `is not`, for instance checks
use `isinstance()`", does identify something we can improve.
This upgrades flake8 in pre-commit and fixes the new warnings.
This bumps the versions of the flake8 plugins specified with pinned
versions as additional dependencies of flake8 for pre-commit.
Doing so gains a warning about a call to warnings.warn with no
stacklevel argument. This appears to be the uncommon case where the
implifit effect of stacklevel=1 is intended, so I have made that
explicit, which clarifies this intent and dismisses the warning.
This expands flake8 linting to include the test suite, and fixes
the resulting warnings. Four code changes are especially notable:
- Unit tests from which documentation is automatically generated
contain a few occurrences of "# @NoEffect". These suppressions
are extended to "# noqa: B015 # @NoEffect" so the corresponding
suppression is also applied for flake8. This is significant
because it actually changes what appears in the code examples in
the generated documentation. However, since the "@NoEffect"
annotation was considered acceptable, this may be okay too. The
resulting examples did not become excessively long.
- Expressions like "[c for c in commits_for_file_generator]" appear
in some unit tests from which documentation is automatically
generated. The simpler form "list(commits_for_file_generator)"
can replace them. This changes the examples in the documentation,
but for the better, since that form is simpler (and also a better
practice in general, thus a better thing to show readers). So I
made those substitutions.
- In test_repo.TestRepo.test_git_work_tree_env, the code intended
to unpatch environment variables after the test was ineffective,
instead causing os.environ to refer to an ordinary dict object
that does not affect environment variables when written to. This
uses unittest.mock.patch.dict instead, so the variables are
unpatched and subsequent writes to environment variables in the
test process are effective.
- In test_submodule.TestSubmodule._do_base_tests, the expression
statement "csm.module().head.ref.tracking_branch() is not None"
appeared to be intended as an assertion, in spite of having been
annoated @NoEffect. This is in light of the context and because,
if the goal were only to exercise the function call, the
"is not None" part would be superfluous. So I made it an
assertion.
This refactors code in the test suite that uses try-finally, to
simplify and clarify what it is doing. An exception to this being
a refactoring is that a possible bug is fixed where a throwaway
environment variable "FOO" was patched for a test and never
unpatched.
There are two patterns refactored here:
- try-(try-except)-finally to try-except-finally. When the entire
suite of the try-block in try-finally is itself a try-except,
that has the same effect as try-except-finally. (Python always
attempts to run the finally suite in either case, and does so
after any applicable except suite is run.)
- Replacing try-finally with an appropriate context manager.
(These changes do not fix, nor introduce, any flake8 errors, but
they were found by manual search for possible problems related to
recent flake8 findings but outside of what flake8 can catch. To
limit scope, this only refactors try-finally in the test suite.)
This clarifies that the object's contents are patched, rather than
patching the attribute used to get the object in the first place.
It is also in keeping with the style of patching os.environ
elsewhere in the test suite.
This was referenced Sep 21, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for this wonderful cleanup! I consider all of these changes improvements, particularly so if they are visible in the documentation.
About @NoEffect, I have no recollection of why this was added and think that less noise is more, if there is a chance to have it removed.
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.)
I am all for ruff :), but am definitely biased as well ;). If you think it will be better, I definitely encourage you to go ahead with it.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
EliahKagan reacted with thumbs up emoji
otc-zuul Bot
pushed a commit
to opentelekomcloud-infra/eyes_on_docs
that referenced
this pull request
Oct 25, 2023
Bump gitpython from 3.1.35 to 3.1.37
Bumps gitpython from 3.1.35 to 3.1.37.
Release notes
Sourced from gitpython's releases.
3.1.37 - a proper fix CVE-2023-41040
What's Changed
Improve Python version and OS compatibility, fixing deprecations by @EliahKagan in gitpython-developers/GitPython#1654
Better document env_case test/fixture and cwd by @EliahKagan in gitpython-developers/GitPython#1657
Remove spurious executable permissions by @EliahKagan in gitpython-developers/GitPython#1658
Fix up checks in Makefile and make them portable by @EliahKagan in gitpython-developers/GitPython#1661
Fix URLs that were redirecting to another license by @EliahKagan in gitpython-developers/GitPython#1662
Assorted small fixes/improvements to root dir docs by @EliahKagan in gitpython-developers/GitPython#1663
Use venv instead of virtualenv in test_installation by @EliahKagan in gitpython-developers/GitPython#1664
Omit py_modules in setup by @EliahKagan in gitpython-developers/GitPython#1665
Don't track code coverage temporary files by @EliahKagan in gitpython-developers/GitPython#1666
Configure tox by @EliahKagan in gitpython-developers/GitPython#1667
Format tests with black and auto-exclude untracked paths by @EliahKagan in gitpython-developers/GitPython#1668
Upgrade and broaden flake8, fixing style problems and bugs by @EliahKagan in gitpython-developers/GitPython#1673
Fix rollback bug in SymbolicReference.set_reference by @EliahKagan in gitpython-developers/GitPython#1675
Remove @NoEffect annotations by @EliahKagan in gitpython-developers/GitPython#1677
Add more checks for the validity of refnames by @facutuesca in gitpython-developers/GitPython#1672
Full Changelog: gitpython-developers/[email protected]
Commits
b27a89f fix makefile to compare commit hashes only
0bd2890 prepare next release
832b6ee remove unnecessary list comprehension to fix CI
e98f57b Merge pull request #1672 from trail-of-forks/robust-refname-checks
1774f1e Merge pull request #1677 from EliahKagan/no-noeffect
a4701a0 Remove @NoEffect annotations
d40320b Merge pull request #1675 from EliahKagan/rollback
d1c1f31 Merge pull request #1673 from EliahKagan/flake8
e480985 Tweak rollback logic in log.to_file
ff84b26 Refactor try-finally cleanup in git/
Additional commits viewable in compare view
Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting @dependabot rebase.
Dependabot commands and options
You can trigger Dependabot actions by commenting on this PR:
@dependabot rebase will rebase this PR
@dependabot recreate will recreate this PR, overwriting any edits that have been made to it
@dependabot merge will merge this PR after your CI passes on it
@dependabot squash and merge will squash and merge this PR after your CI passes on it
@dependabot cancel merge will cancel a previously requested merge and block automerging
@dependabot reopen will reopen this PR if it is closed
@dependabot close will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
@dependabot show <dependency name> ignore conditions will show all of the ignore conditions of the specified dependency
@dependabot ignore this major version will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
@dependabot ignore this minor version will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
@dependabot ignore this dependency will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
You can disable automated security fix PRs for this repo from the Security Alerts page.
Reviewed-by: Vladimir Vshivkov
Reviewers
Byron
Byron approved these changes
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
|
%5D(https%3A%2F%2Frenovatebot.com){kind=link}