Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork938
Upgrade and broaden flake8, fixing style problems and bugs#1673
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Merged
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This file contains hidden or 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 pycodestyledependency to be upgraded from 2.10.* to 2.11.*, which is desirablebecause:- 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 pinnedversions as additional dependencies of flake8 for pre-commit.Doing so gains a warning about a call to warnings.warn with nostacklevel argument. This appears to be the uncommon case where theimplifit effect of stacklevel=1 is intended, so I have made thatexplicit, which clarifies this intent and dismisses the warning.
This expands flake8 linting to include the test suite, and fixesthe 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, tosimplify and clarify what it is doing. An exception to this beinga refactoring is that a possible bug is fixed where a throwawayenvironment variable "FOO" was patched for a test and neverunpatched.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, butthey were found by manual search for possible problems related torecent flake8 findings but outside of what flake8 can catch. Tolimit scope, this only refactors try-finally in the test suite.)
This clarifies that the object's contents are patched, rather thanpatching the attribute used to get the object in the first place.It is also in keeping with the style of patching os.environelsewhere in the test suite.
This was referencedSep 21, 2023
Byron approved these changesSep 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
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 forruff :), but am definitely biased as well ;). If you think it will be better, I definitely encourage you to go ahead with it.
renovatebot referenced this pull request in allenporter/flux-localSep 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 fixCVE-2023-41040[CompareSource](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[https://github.com/gitpython-developers/GitPython/pull/1654](https://togithub.com/gitpython-developers/GitPython/pull/1654)- Better document env_case test/fixture and cwd by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1657](https://togithub.com/gitpython-developers/GitPython/pull/1657)- Remove spurious executable permissions by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1658](https://togithub.com/gitpython-developers/GitPython/pull/1658)- Fix up checks in Makefile and make them portable by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1661](https://togithub.com/gitpython-developers/GitPython/pull/1661)- Fix URLs that were redirecting to another license by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1662](https://togithub.com/gitpython-developers/GitPython/pull/1662)- Assorted small fixes/improvements to root dir docs by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1663](https://togithub.com/gitpython-developers/GitPython/pull/1663)- Use venv instead of virtualenv in test_installation by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1664](https://togithub.com/gitpython-developers/GitPython/pull/1664)- Omit py_modules in setup by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1665](https://togithub.com/gitpython-developers/GitPython/pull/1665)- Don't track code coverage temporary files by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1666](https://togithub.com/gitpython-developers/GitPython/pull/1666)- Configure tox by [@​EliahKagan](https://togithub.com/EliahKagan)in[https://github.com/gitpython-developers/GitPython/pull/1667](https://togithub.com/gitpython-developers/GitPython/pull/1667)- Format tests with black and auto-exclude untracked paths by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1668](https://togithub.com/gitpython-developers/GitPython/pull/1668)- Upgrade and broaden flake8, fixing style problems and bugs by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1673](https://togithub.com/gitpython-developers/GitPython/pull/1673)- Fix rollback bug in SymbolicReference.set_reference by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1675](https://togithub.com/gitpython-developers/GitPython/pull/1675)- Remove `@NoEffect` annotations by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1677](https://togithub.com/gitpython-developers/GitPython/pull/1677)- Add more checks for the validity of refnames by[@​facutuesca](https://togithub.com/facutuesca) in[https://github.com/gitpython-developers/GitPython/pull/1672](https://togithub.com/gitpython-developers/GitPython/pull/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 therebase/retry checkbox.🔕 **Ignore**: Close this PR and you won't be reminded about this updateagain.---- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, checkthis box---This PR has been generated by [MendRenovate](https://www.mend.io/free-developer-tools/renovate/). Viewrepository 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>
%5d(https%3a%2f%2frenovatebot.com){kind=link}
otc-zuulbot pushed a commit to opentelekomcloud-infra/eyes_on_docs that referenced this pull requestOct 25, 2023
Bump gitpython from 3.1.35 to 3.1.37Bumps gitpython from 3.1.35 to 3.1.37.Release notesSourced from gitpython's releases.3.1.37 - a proper fixCVE-2023-41040What's ChangedImprove Python version and OS compatibility, fixing deprecations by @EliahKagan ingitpython-developers/GitPython#1654Better document env_case test/fixture and cwd by @EliahKagan ingitpython-developers/GitPython#1657Remove spurious executable permissions by @EliahKagan ingitpython-developers/GitPython#1658Fix up checks in Makefile and make them portable by @EliahKagan ingitpython-developers/GitPython#1661Fix URLs that were redirecting to another license by @EliahKagan ingitpython-developers/GitPython#1662Assorted small fixes/improvements to root dir docs by @EliahKagan ingitpython-developers/GitPython#1663Use venv instead of virtualenv in test_installation by @EliahKagan ingitpython-developers/GitPython#1664Omit py_modules in setup by @EliahKagan ingitpython-developers/GitPython#1665Don't track code coverage temporary files by @EliahKagan ingitpython-developers/GitPython#1666Configure tox by @EliahKagan ingitpython-developers/GitPython#1667Format tests with black and auto-exclude untracked paths by @EliahKagan ingitpython-developers/GitPython#1668Upgrade and broaden flake8, fixing style problems and bugs by @EliahKagan ingitpython-developers/GitPython#1673Fix rollback bug in SymbolicReference.set_reference by @EliahKagan ingitpython-developers/GitPython#1675Remove@NoEffect annotations by @EliahKagan ingitpython-developers/GitPython#1677Add more checks for the validity of refnames by @facutuesca ingitpython-developers/GitPython#1672Full Changelog: gitpython-developers/GitPython@3.1.36...3.1.37Commitsb27a89f fix makefile to compare commit hashes only0bd2890 prepare next release832b6ee remove unnecessary list comprehension to fix CIe98f57b Merge pull request #1672 from trail-of-forks/robust-refname-checks1774f1e Merge pull request #1677 from EliahKagan/no-noeffecta4701a0 Remove@NoEffect annotationsd40320b Merge pull request #1675 from EliahKagan/rollbackd1c1f31 Merge pull request #1673 from EliahKagan/flake8e480985 Tweak rollback logic in log.to_fileff84b26 Refactor try-finally cleanup in git/Additional commits viewable in compare viewDependabot 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 optionsYou 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 freeto join this conversation on GitHub. Already have an account?Sign in to comment
Labels
None yet
2 participants
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.
Uh oh!
There was an error while loading.Please reload this page.
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 appearedexact 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.)