Upgrade and broaden flake8, fixing style problems and bugs #1673
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
Byron
approved these changes
Sep 21, 2023
There was a problem hiding this comment.
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
flake8in this project withruff, which is modern, versatile, and extremely fast. (This is also why I have not proposed that we also adoptisort, even thoughisortis one of my favorite tools:ruffmight 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.
renovate bot
added a commit
to allenporter/flux-local
that referenced
this pull request
Sep 23, 2023
[](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [GitPython](https://togithub.com/gitpython-developers/GitPython) | `==3.1.36` -> `==3.1.37` | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>gitpython-developers/GitPython (GitPython)</summary> ### [`v3.1.37`](https://togithub.com/gitpython-developers/GitPython/releases/tag/3.1.37): - a proper fix CVE-2023-41040 [Compare Source](https://togithub.com/gitpython-developers/GitPython/compare/3.1.36...3.1.37) #### What's Changed - Improve Python version and OS compatibility, fixing deprecations by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1654 - Better document env_case test/fixture and cwd by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1657 - Remove spurious executable permissions by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1658 - Fix up checks in Makefile and make them portable by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1661 - Fix URLs that were redirecting to another license by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1662 - Assorted small fixes/improvements to root dir docs by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1663 - Use venv instead of virtualenv in test_installation by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1664 - Omit py_modules in setup by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1665 - Don't track code coverage temporary files by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1666 - Configure tox by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1667 - Format tests with black and auto-exclude untracked paths by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1668 - Upgrade and broaden flake8, fixing style problems and bugs by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1673 - Fix rollback bug in SymbolicReference.set_reference by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1675 - Remove `@NoEffect` annotations by [@​EliahKagan](https://togithub.com/EliahKagan) in [gitpython-developers/GitPython#1677 - Add more checks for the validity of refnames by [@​facutuesca](https://togithub.com/facutuesca) in [gitpython-developers/GitPython#1672 **Full Changelog**: gitpython-developers/GitPython@3.1.36...3.1.37 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/allenporter/flux-local). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi45Ny4xIiwidXBkYXRlZEluVmVyIjoiMzYuOTcuMSIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==--> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
otc-zuul bot
pushed a commit
to opentelekomcloud-infra/grafana-docs-monitoring
that referenced
this pull request
Oct 25, 2023
](https://renovatebot.com){kind=link}
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/GitPython@3.1.36...3.1.37 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #1670
Fixes #1671
I've upgraded
flake8in the pre-commit configuration, which automatically upgrades itspycodestyledependency. 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 ofpycodestyleand new versions of the non-default packages. This included changing==toisand!=tois notwhen comparingtypeobjects, since in all cases it appeared exact type matching was intended (rather than what anissubclassor, under simplification of the surrounding code,isinstancewould check). See #1670.I've also broadened
flake8to check the whole project (except thedoc/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 usingunittest.mock.patch.dict, which as noted there is okay because it is only in the tests). I then looked for bugs infinallycleanup logic throughout the project, in case anything else like that was present that I could identify and fix. I found no further serious bugs insidetest/while doing this, but I did find some areas I could simplify or otherwise improve, including one place where a throwaway environment variableFOOwas never unpatched.I included those changes in this PR, but not any changes to code in
git/other than to fix whatflake8found. One place ingit/that should be changed is a bug that merits fixing in some way, which I opened #1669 for. Other areas of improvement ingit/related to the use offinallyare 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 ingit/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:
@NoEffectappeared 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@NoEffectis no longer needed, then I think configuringflake8to disable the specific warning in those two specific test modules would be better, since then neither@NoEffectnor# noqa: B015would need to appear anywhere in documentation example code.[c for c in commits_for_file_generator]should belist(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
noqacomments, 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 withflake8--for example, by adding it and its extra plugins as development dependencies of the project (they are still only installed by and forpre-commit), running it on CI, enabling more checks, or attempting to get theflake8-type-checkingplugin listed inrequirements-dev.txtworking. The reason is that I think it would be beneficial to replaceflake8in this project withruff, which is modern, versatile, and extremely fast. (This is also why I have not proposed that we also adoptisort, even thoughisortis one of my favorite tools:ruffmight take care of that, too.)