Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade and broaden flake8, fixing style problems and bugs #1673

Merged
merged 5 commits into from
Sep 21, 2023

Conversation

EliahKagan
Copy link
Contributor

@EliahKagan EliahKagan commented Sep 21, 2023

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.)

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.
Copy link
Member

@Byron Byron left a 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.

@Byron Byron merged commit d1c1f31 into gitpython-developers:main Sep 21, 2023
8 checks passed
@Byron Byron added this to the v3.1.37 - Bugfixes milestone Sep 21, 2023
@EliahKagan EliahKagan deleted the flake8 branch September 21, 2023 18:58
renovate bot referenced this pull request in allenporter/flux-local Sep 23, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](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` |
[![age](https://developer.mend.io/api/mc/badges/age/pypi/GitPython/3.1.37?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/pypi/GitPython/3.1.37?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/pypi/GitPython/3.1.36/3.1.37?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/pypi/GitPython/3.1.36/3.1.37?slim=true)](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
[@&#8203;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
[@&#8203;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
[@&#8203;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
[@&#8203;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
[@&#8203;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
[@&#8203;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
[@&#8203;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
[@&#8203;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
[@&#8203;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 [@&#8203;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
[@&#8203;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
[@&#8203;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
[@&#8203;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
[@&#8203;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
[@&#8203;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 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/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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants