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

Omit warning prefix in "Bad git executable" message #1816

Merged
merged 5 commits into from
Feb 6, 2024

Conversation

EliahKagan
Copy link
Contributor

@EliahKagan EliahKagan commented Feb 6, 2024

This builds on the changes in #1815 by omitting the WARNING: prefix from the message itself that is produced when GIT_PYTHON_REFRESH is set to warn or a synonym (such as w or log) and the initial refresh performed at import time fails. This is the change suggested in #1815 (comment).

When logging is not configured, this changes what we see when the initial refresh fails:

-WARNING: Bad git executable.
+Bad git executable.

This may be considered slightly worse than before, but the advantage is that it doesn't print something that looks like a severity (or a redundancy) when logging is configured. For example, with common logging configuration such as by logging.basicConfig(), we get this improvement:

-CRITICAL:git.cmd:WARNING: Bad git executable.
+CRITICAL:git.cmd:Bad git executable.

This also makes some improvements to the tests. Besides updating them for the changed message (3250313), it:

  • Test that the GitCommandNotFound exception raised when attempting a git operation, when the command in Git.GIT_PYTHON_GIT_EXECUTABLE does not exist but was set directly (rather than through the environment variable or an explicit call to one of the refresh functions), has an expected command attribute value (85ef145). This case has been under test for a long time, but the test had not checked that attribute.
  • Reorder switch the order of that and an adjacent test so the tests of behavior that directly involves GIT_PYTHON_GIT_EXECUTABLE are all together (also in 85ef145).
  • Use Git consistently in test_git.py, instead of using Git in some places and type(self.git) in others. See db36174 for the rationale.
  • Change a call like _logger.warning("%s", X) to _logger.warning(X) in test_index.py, because GitPython uses the latter in all the other places where they would be equivalent. See 4b86993 for details. (This is relevant to this PR because 5faf621 adds another such call.)

Some tests in test_git.TestGit used type(self.git) while others
used Git. This changes them all to use Git. The distinction could
be relevant if self.git were set as a mock, but this is not being
done, and if it were, it is not obvious which would be preferred.
The intention of the existing tests is to test attributes of the
Git class, so this picks that simpler expression.
This changes test_initial_refresh_from_bad_git_path_env_warn to
assert a message with no added "WARNING:" prefix. As discussed
in gitpython-developers#1815, the extra prefix is confusing with logging configured,
showing "CRITICAL:git.cmd:WARNING: Bad git executable." instead of
"CRITICAL:git.cmd: Bad git executable."
As tested for in 3250313. This makes the test pass.
When logging with only a msg argument, it is a full literal message
rather than a format string (as it is when there are placeholders).
Thus both `...("%s", text)` and `...(text)`, where `...` is a
logging method or function, are equally good code styles, provided
`text` really is known to behave the same as `str(text)`.

The latter style, `...(text)`, was used in all logging calls, both
in the git module and in the test suite, except one. This changes
the one outlier from `...("%s", text)` to `...(text)` for stylistic
consistency and to avoid giving the false impression that there is
something special about that call.
@EliahKagan EliahKagan marked this pull request as ready for review February 6, 2024 08:57
@Byron
Copy link
Member

Byron commented Feb 6, 2024

Thanks a lot for your continued support!

I particularly like how diffs are used to visualise the effective changes, they really help to make the changes pop.

@Byron Byron merged commit 3749037 into gitpython-developers:main Feb 6, 2024
22 checks passed
@EliahKagan EliahKagan deleted the refresh-format branch February 6, 2024 18:03
lettuce-bot bot added a commit to lettuce-financial/github-bot-signed-commit that referenced this pull request Feb 15, 2024
[![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://github.com/gitpython-developers/GitPython) |
`==3.1.41` -> `==3.1.42` |
[![age](https://developer.mend.io/api/mc/badges/age/pypi/GitPython/3.1.42?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/pypi/GitPython/3.1.42?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/pypi/GitPython/3.1.41/3.1.42?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/pypi/GitPython/3.1.41/3.1.42?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>gitpython-developers/GitPython (GitPython)</summary>

###
[`v3.1.42`](https://github.com/gitpython-developers/GitPython/releases/tag/3.1.42)

[Compare
Source](https://github.com/gitpython-developers/GitPython/compare/3.1.41...3.1.42)

#### What's Changed

- Fix release link in changelog by
[@&#8203;PeterJCLaw](https://github.com/PeterJCLaw) in
[gitpython-developers/GitPython#1795
- Remove test dependency on sumtypes library by
[@&#8203;EliahKagan](https://github.com/EliahKagan) in
[gitpython-developers/GitPython#1798
- Pin Sphinx plugins to compatible versions by
[@&#8203;EliahKagan](https://github.com/EliahKagan) in
[gitpython-developers/GitPython#1803
- fix: treeNotSorted issue by
[@&#8203;et-repositories](https://github.com/et-repositories) in
[gitpython-developers/GitPython#1799
- Remove git.util.NullHandler by
[@&#8203;EliahKagan](https://github.com/EliahKagan) in
[gitpython-developers/GitPython#1807
- Clarify why GIT_PYTHON_GIT_EXECUTABLE may be set on failure by
[@&#8203;EliahKagan](https://github.com/EliahKagan) in
[gitpython-developers/GitPython#1810
- Report actual attempted Git command when Git.refresh fails by
[@&#8203;EliahKagan](https://github.com/EliahKagan) in
[gitpython-developers/GitPython#1812
- Don't suppress messages when logging is not configured by
[@&#8203;EliahKagan](https://github.com/EliahKagan) in
[gitpython-developers/GitPython#1813
- Pin Python 3.9.16 on Cygwin CI by
[@&#8203;EliahKagan](https://github.com/EliahKagan) in
[gitpython-developers/GitPython#1814
- Have initial refresh use a logger to warn by
[@&#8203;EliahKagan](https://github.com/EliahKagan) in
[gitpython-developers/GitPython#1815
- Omit warning prefix in "Bad git executable" message by
[@&#8203;EliahKagan](https://github.com/EliahKagan) in
[gitpython-developers/GitPython#1816
- Test with M1 macOS CI runner by
[@&#8203;EliahKagan](https://github.com/EliahKagan) in
[gitpython-developers/GitPython#1817
- Bump pre-commit/action from 3.0.0 to 3.0.1 by
[@&#8203;dependabot](https://github.com/dependabot) in
[gitpython-developers/GitPython#1818
- Bump Vampire/setup-wsl from 2.0.2 to 3.0.0 by
[@&#8203;dependabot](https://github.com/dependabot) in
[gitpython-developers/GitPython#1819
- Remove deprecated section in README.md by
[@&#8203;marcm-ml](https://github.com/marcm-ml) in
[gitpython-developers/GitPython#1823
- Keep temp files out of project dir and improve cleanup by
[@&#8203;EliahKagan](https://github.com/EliahKagan) in
[gitpython-developers/GitPython#1825

#### New Contributors

- [@&#8203;PeterJCLaw](https://github.com/PeterJCLaw) made their first
contribution in
[gitpython-developers/GitPython#1795
- [@&#8203;et-repositories](https://github.com/et-repositories) made
their first contribution in
[gitpython-developers/GitPython#1799
- [@&#8203;marcm-ml](https://github.com/marcm-ml) made their first
contribution in
[gitpython-developers/GitPython#1823

**Full Changelog**:
gitpython-developers/GitPython@3.1.41...3.1.42

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about these
updates 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/lettuce-financial/github-bot-signed-commit).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4xNzMuMCIsInVwZGF0ZWRJblZlciI6IjM3LjE3My4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->
renovate bot added a commit to allenporter/flux-local that referenced this pull request Feb 16, 2024
[![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://github.com/gitpython-developers/GitPython) |
`==3.1.41` -> `==3.1.42` |
[![age](https://developer.mend.io/api/mc/badges/age/pypi/GitPython/3.1.42?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/pypi/GitPython/3.1.42?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/pypi/GitPython/3.1.41/3.1.42?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/pypi/GitPython/3.1.41/3.1.42?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>gitpython-developers/GitPython (GitPython)</summary>

###
[`v3.1.42`](https://github.com/gitpython-developers/GitPython/releases/tag/3.1.42)

[Compare
Source](https://github.com/gitpython-developers/GitPython/compare/3.1.41...3.1.42)

#### What's Changed

- Fix release link in changelog by
[@&#8203;PeterJCLaw](https://github.com/PeterJCLaw) in
[gitpython-developers/GitPython#1795
- Remove test dependency on sumtypes library by
[@&#8203;EliahKagan](https://github.com/EliahKagan) in
[gitpython-developers/GitPython#1798
- Pin Sphinx plugins to compatible versions by
[@&#8203;EliahKagan](https://github.com/EliahKagan) in
[gitpython-developers/GitPython#1803
- fix: treeNotSorted issue by
[@&#8203;et-repositories](https://github.com/et-repositories) in
[gitpython-developers/GitPython#1799
- Remove git.util.NullHandler by
[@&#8203;EliahKagan](https://github.com/EliahKagan) in
[gitpython-developers/GitPython#1807
- Clarify why GIT_PYTHON_GIT_EXECUTABLE may be set on failure by
[@&#8203;EliahKagan](https://github.com/EliahKagan) in
[gitpython-developers/GitPython#1810
- Report actual attempted Git command when Git.refresh fails by
[@&#8203;EliahKagan](https://github.com/EliahKagan) in
[gitpython-developers/GitPython#1812
- Don't suppress messages when logging is not configured by
[@&#8203;EliahKagan](https://github.com/EliahKagan) in
[gitpython-developers/GitPython#1813
- Pin Python 3.9.16 on Cygwin CI by
[@&#8203;EliahKagan](https://github.com/EliahKagan) in
[gitpython-developers/GitPython#1814
- Have initial refresh use a logger to warn by
[@&#8203;EliahKagan](https://github.com/EliahKagan) in
[gitpython-developers/GitPython#1815
- Omit warning prefix in "Bad git executable" message by
[@&#8203;EliahKagan](https://github.com/EliahKagan) in
[gitpython-developers/GitPython#1816
- Test with M1 macOS CI runner by
[@&#8203;EliahKagan](https://github.com/EliahKagan) in
[gitpython-developers/GitPython#1817
- Bump pre-commit/action from 3.0.0 to 3.0.1 by
[@&#8203;dependabot](https://github.com/dependabot) in
[gitpython-developers/GitPython#1818
- Bump Vampire/setup-wsl from 2.0.2 to 3.0.0 by
[@&#8203;dependabot](https://github.com/dependabot) in
[gitpython-developers/GitPython#1819
- Remove deprecated section in README.md by
[@&#8203;marcm-ml](https://github.com/marcm-ml) in
[gitpython-developers/GitPython#1823
- Keep temp files out of project dir and improve cleanup by
[@&#8203;EliahKagan](https://github.com/EliahKagan) in
[gitpython-developers/GitPython#1825

#### New Contributors

- [@&#8203;PeterJCLaw](https://github.com/PeterJCLaw) made their first
contribution in
[gitpython-developers/GitPython#1795
- [@&#8203;et-repositories](https://github.com/et-repositories) made
their first contribution in
[gitpython-developers/GitPython#1799
- [@&#8203;marcm-ml](https://github.com/marcm-ml) made their first
contribution in
[gitpython-developers/GitPython#1823

**Full Changelog**:
gitpython-developers/GitPython@3.1.41...3.1.42

</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:eyJjcmVhdGVkSW5WZXIiOiIzNy4xNzMuMCIsInVwZGF0ZWRJblZlciI6IjM3LjE3My4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants