Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork937
Have initial refresh use a logger to warn#1815
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This adds tests like the existing ones but for when git.refresh iscalled with no arguments and the path is provided as the value ofthe GIT_PYTHON_GIT_EXECUTABLE environment variable.The preexisting tests, which this retains unchanged except withslightly more specific names to avoid confusion with the new tests,are of git.refresh(path).One benefit of these tests is to make a subtle but important aspectof the established behavior clear: relative paths are immediatelyresolved when passed as a path argument, but they are kept relativewhen given as the value of the environment variable.
This adds tests of the initial refresh that is attemptedautomatically on import. All the refresh tests prior to this pointtest subsequent refreshes. Those tests are kept, and new ones areadded that simulate the condition of not having yet done theinitial refresh by setting Git.GIT_PYTHON_GIT_EXECUTABLE to None.Some current behavior these tests assert may change forgitpython-developers#1808.
This is instead of the current behavior writing the message tostdout.This commit does not change the behavior of the code under test,but it changes tests to assert the following:- "Bad git executable" messages are logged, at level CRITICAL.- "log" (and "l") is recognized as another synonym of "warn".- "silent" is recognized as a synonym of "quiet" (as "silence" is).Although it is ambiguous whether this should be logged at levelERROR or CRITICAL, because git.refresh is still expected to beusable and can be called manually, not having a working git is acondition in which GitPython, and any program that really relies onits functionality, should be expected not work. That is the generalrationale for using CRIICAL here. There are also two specificreasons:- Existing messages GitPython logs as ERROR typically represent errors in individual operations on repositories, which could fail without indicating that GitPython's overall functionality is in an unusable state. Using the higher CRITICAL level for this situation makes sense for contrast.- Prior togitpython-developers#1813, logging messsges emitted from GitPython modules, no matter the level, were suppressed when logging was not configured, but because this message was printed instead of being logged, it was still shown. Now that it is to be logged, there may be a benefit to have an easy way for someone to bring back a close approximation of the old behavior. Having this message be at a higher logging level makes that easier to do. (This is a less important reason, as that should rarely be done.)test_initial_refresh_from_bad_git_path_env_warn is the main changedtest. All tests should pass again once code is changed forgitpython-developers#1808.
Forgitpython-developers#1808.Seeac50d8e for details on the changes.
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 so much! With this PR it seems reasonable to claim that the messaging in GitPython is finally consistent. Everything useslogging, and messages are no suppressed by default.
I particularly love that you managed to do test-driven development here, that's the way (for those who have the experience to set up such tests in the first place).
Admission: I only skimmed the tests themselves due to my own state of mind and the trust in the PRs author.
Although it's not really an inconsistency in the sense that you mean, I should mention that the way the message is prefixed is imperfect. I am unsure if there is a better way. The message itself begins, as before: When logging is not configured, That's a bit weird, but I'm unsure if anything should be done to change it. I don't recommend having the message itself differ based on whether or not or how logging is configured. But maybe
I plan to look for possible remaining inconsistencies, so I'll try to either comment here about not finding them, or open one or more issues or PRs for any that I do find.
I agree. In particular, I looked for other
The tests contain some code duplication. The ways of reducing it that I thought of didn't seem like they would necessarily be improvements, though future changes might worsen the situation and make it more important to extract shared logic, if it becomes important to test more combinations of environment variables. I bring this up only in case you already happened to notice that the duplication, or volume of the new tests, made them less scrutable than ideal. If that was the case, I can take another look at them. |
My intuition here is to remove it, and allow
I would not suggest to spend time on any deduplication, it was just me feeling a bit rushed while still thinking the PR should be merged, all things considered. |
This changes test_initial_refresh_from_bad_git_path_env_warn toassert a message with no added "WARNING:" prefix. As discussedingitpython-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."
The one other inconsistency I found in the messages is that the git command used is shown only in the exception message on a subsequent refresh, butnot shown in exception message from an initial refresh or in the logged message on an initial refresh in I am unsure if this should really be changed or if it even really is a real inconsistency. I suspect the reason it was not included was that there was never originally a goal to include it anywhere, which may also partially explain#1809. Even though#1809 is fixed by the changes in#1812, I don't think this necessarily means the other messages--which are not in any way inaccurate or misleading--need to be expanded to include that information. Other than the above, I can think of two other reasons the information may not be included in those messages, one of which is obsolete but the other of which may still be applicable:
No problem! If we end up showing the command in more messages as discussed above, then the duplication in the tests (to test such a change) might become worse, in which case I'll take another look. Otherwise I won't worry about it.
I've made that change in#1816. |
[](https://renovatebot.com)This PR contains the following updates:| Package | Change | Age | Adoption | Passing | Confidence ||---|---|---|---|---|---|| [GitPython](https://togithub.com/gitpython-developers/GitPython) |`==3.1.41` -> `==3.1.42` |[](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.42`](https://togithub.com/gitpython-developers/GitPython/releases/tag/3.1.42)[CompareSource](https://togithub.com/gitpython-developers/GitPython/compare/3.1.41...3.1.42)#### What's Changed- Fix release link in changelog by[@​PeterJCLaw](https://togithub.com/PeterJCLaw) in[https://github.com/gitpython-developers/GitPython/pull/1795](https://togithub.com/gitpython-developers/GitPython/pull/1795)- Remove test dependency on sumtypes library by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1798](https://togithub.com/gitpython-developers/GitPython/pull/1798)- Pin Sphinx plugins to compatible versions by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1803](https://togithub.com/gitpython-developers/GitPython/pull/1803)- fix: treeNotSorted issue by[@​et-repositories](https://togithub.com/et-repositories) in[https://github.com/gitpython-developers/GitPython/pull/1799](https://togithub.com/gitpython-developers/GitPython/pull/1799)- Remove git.util.NullHandler by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1807](https://togithub.com/gitpython-developers/GitPython/pull/1807)- Clarify why GIT_PYTHON_GIT_EXECUTABLE may be set on failure by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1810](https://togithub.com/gitpython-developers/GitPython/pull/1810)- Report actual attempted Git command when Git.refresh fails by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1812](https://togithub.com/gitpython-developers/GitPython/pull/1812)- Don't suppress messages when logging is not configured by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1813](https://togithub.com/gitpython-developers/GitPython/pull/1813)- Pin Python 3.9.16 on Cygwin CI by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1814](https://togithub.com/gitpython-developers/GitPython/pull/1814)- Have initial refresh use a logger to warn by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1815](https://togithub.com/gitpython-developers/GitPython/pull/1815)- Omit warning prefix in "Bad git executable" message by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1816](https://togithub.com/gitpython-developers/GitPython/pull/1816)- Test with M1 macOS CI runner by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1817](https://togithub.com/gitpython-developers/GitPython/pull/1817)- Bump pre-commit/action from 3.0.0 to 3.0.1 by[@​dependabot](https://togithub.com/dependabot) in[https://github.com/gitpython-developers/GitPython/pull/1818](https://togithub.com/gitpython-developers/GitPython/pull/1818)- Bump Vampire/setup-wsl from 2.0.2 to 3.0.0 by[@​dependabot](https://togithub.com/dependabot) in[https://github.com/gitpython-developers/GitPython/pull/1819](https://togithub.com/gitpython-developers/GitPython/pull/1819)- Remove deprecated section in README.md by[@​marcm-ml](https://togithub.com/marcm-ml) in[https://github.com/gitpython-developers/GitPython/pull/1823](https://togithub.com/gitpython-developers/GitPython/pull/1823)- Keep temp files out of project dir and improve cleanup by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1825](https://togithub.com/gitpython-developers/GitPython/pull/1825)#### New Contributors- [@​PeterJCLaw](https://togithub.com/PeterJCLaw) made their firstcontribution in[https://github.com/gitpython-developers/GitPython/pull/1795](https://togithub.com/gitpython-developers/GitPython/pull/1795)- [@​et-repositories](https://togithub.com/et-repositories) madetheir first contribution in[https://github.com/gitpython-developers/GitPython/pull/1799](https://togithub.com/gitpython-developers/GitPython/pull/1799)- [@​marcm-ml](https://togithub.com/marcm-ml) made their firstcontribution in[https://github.com/gitpython-developers/GitPython/pull/1823](https://togithub.com/gitpython-developers/GitPython/pull/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 youare satisfied.♻ **Rebasing**: Whenever PR becomes conflicted, or you tick therebase/retry checkbox.🔕 **Ignore**: Close this PR and you won't be reminded about theseupdates again.---- [ ] <!-- 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/lettuce-financial/github-bot-signed-commit).<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4xNzMuMCIsInVwZGF0ZWRJblZlciI6IjM3LjE3My4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->
%5d(https%3a%2f%2frenovatebot.com){kind=link}
[](https://renovatebot.com)This PR contains the following updates:| Package | Change | Age | Adoption | Passing | Confidence ||---|---|---|---|---|---|| [GitPython](https://togithub.com/gitpython-developers/GitPython) |`==3.1.41` -> `==3.1.42` |[](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.42`](https://togithub.com/gitpython-developers/GitPython/releases/tag/3.1.42)[CompareSource](https://togithub.com/gitpython-developers/GitPython/compare/3.1.41...3.1.42)#### What's Changed- Fix release link in changelog by[@​PeterJCLaw](https://togithub.com/PeterJCLaw) in[https://github.com/gitpython-developers/GitPython/pull/1795](https://togithub.com/gitpython-developers/GitPython/pull/1795)- Remove test dependency on sumtypes library by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1798](https://togithub.com/gitpython-developers/GitPython/pull/1798)- Pin Sphinx plugins to compatible versions by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1803](https://togithub.com/gitpython-developers/GitPython/pull/1803)- fix: treeNotSorted issue by[@​et-repositories](https://togithub.com/et-repositories) in[https://github.com/gitpython-developers/GitPython/pull/1799](https://togithub.com/gitpython-developers/GitPython/pull/1799)- Remove git.util.NullHandler by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1807](https://togithub.com/gitpython-developers/GitPython/pull/1807)- Clarify why GIT_PYTHON_GIT_EXECUTABLE may be set on failure by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1810](https://togithub.com/gitpython-developers/GitPython/pull/1810)- Report actual attempted Git command when Git.refresh fails by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1812](https://togithub.com/gitpython-developers/GitPython/pull/1812)- Don't suppress messages when logging is not configured by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1813](https://togithub.com/gitpython-developers/GitPython/pull/1813)- Pin Python 3.9.16 on Cygwin CI by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1814](https://togithub.com/gitpython-developers/GitPython/pull/1814)- Have initial refresh use a logger to warn by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1815](https://togithub.com/gitpython-developers/GitPython/pull/1815)- Omit warning prefix in "Bad git executable" message by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1816](https://togithub.com/gitpython-developers/GitPython/pull/1816)- Test with M1 macOS CI runner by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1817](https://togithub.com/gitpython-developers/GitPython/pull/1817)- Bump pre-commit/action from 3.0.0 to 3.0.1 by[@​dependabot](https://togithub.com/dependabot) in[https://github.com/gitpython-developers/GitPython/pull/1818](https://togithub.com/gitpython-developers/GitPython/pull/1818)- Bump Vampire/setup-wsl from 2.0.2 to 3.0.0 by[@​dependabot](https://togithub.com/dependabot) in[https://github.com/gitpython-developers/GitPython/pull/1819](https://togithub.com/gitpython-developers/GitPython/pull/1819)- Remove deprecated section in README.md by[@​marcm-ml](https://togithub.com/marcm-ml) in[https://github.com/gitpython-developers/GitPython/pull/1823](https://togithub.com/gitpython-developers/GitPython/pull/1823)- Keep temp files out of project dir and improve cleanup by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1825](https://togithub.com/gitpython-developers/GitPython/pull/1825)#### New Contributors- [@​PeterJCLaw](https://togithub.com/PeterJCLaw) made their firstcontribution in[https://github.com/gitpython-developers/GitPython/pull/1795](https://togithub.com/gitpython-developers/GitPython/pull/1795)- [@​et-repositories](https://togithub.com/et-repositories) madetheir first contribution in[https://github.com/gitpython-developers/GitPython/pull/1799](https://togithub.com/gitpython-developers/GitPython/pull/1799)- [@​marcm-ml](https://togithub.com/marcm-ml) made their firstcontribution in[https://github.com/gitpython-developers/GitPython/pull/1823](https://togithub.com/gitpython-developers/GitPython/pull/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 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:eyJjcmVhdGVkSW5WZXIiOiIzNy4xNzMuMCIsInVwZGF0ZWRJblZlciI6IjM3LjE3My4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
The fragment of the refresh failure message (which is often writtento a terminal) about the effect of setting GIT_PYTHON_REFRESH tocontrol refesh failure reporting had previously been formatted witha maximum line length of 79 columns--in the message itself, not thecode for the message--so that it would fit in a traditional 80column wide terminal. This remains one of the popular widths to setfor terminal windows in a GUI, so it seems worthwhile to preserve.In3a6e3ef (gitpython-developers#1815), I had inadvertently made the line one charactertoo long for that; at 80 columns, it would cause the newline to bewritten at the start of the next line, creating an unsightly extraline break.This is pretty minor, but what seems to me like an equally goodalternative wording avoids it, so this commit shortens the wording.
The fragment of the refresh failure message (which is often writtento a terminal) about the effect of setting GIT_PYTHON_REFRESH tocontrol refesh failure reporting had previously been formatted witha maximum line length of 79 columns--in the message itself, not thecode for the message--so that it would fit in a traditional 80column wide terminal. This remains one of the popular widths to setfor terminal windows in a GUI, so it seems worthwhile to preserve.In3a6e3ef (gitpython-developers#1815), I had inadvertently made the line one charactertoo long for that; at 80 columns, it would cause the newline to bewritten at the start of the next line, creating an unsightly extraline break.This is pretty minor, but what seems to me like an equally goodalternative wording avoids it, so this commit shortens the wording.
Uh oh!
There was an error while loading.Please reload this page.
Fixes#1808
Code changes in the
gitmodule:When the initial call to
git.refreshthat happens on import fails to find agitcommand and is configured to warn, it warns using a logger rather than withprint. Although this is conceptually a warning, it is emitted with levelCRTICAL, for the reasons given inac50d8e (the commit in this PR that changes the tests for it).The message is changed to document this.
A few more values, for existing
GIT_PYTHON_REFRESHmodes, are added as synonyms. No new modes are added, nor are their states expanded. The motivation for adding new synonyms is sologachieves logging, i.e., it is another way to saywarn. Butlis also added as its short form. (This might ordinarily be objected to on the grounds that, when written asland not the also-recognizedL, it could be confused with1. Fortunately1also meanswarn, so1andlwill have the same meaning anyway.) Furthermore,silentis added as a synonym ofquiet, sincesilenceis already recognized for that, sosilentseems likely to be used by accident anyway.The reason I have not gone further at this time (as was discussed inthe second part of this comment and inthis reply) is it can be made more configurable in the future if it looks like that is valuable, but removing existing configurability could break more uses. Furthermore, because the level is
CRITICALwhich differs from that of any other GitPython messages, the better approach of controlling the effect in an application's own logging configuration may be sufficient.Test changes:
Rename some existing tests more specifically, so they are not confused with new ones. These are the tests of subsequent (i.e., non-initial) calls to
git.refreshwhen an explicitpathargument is passed.Test subsequent refreshes performed with no arguments. That is, test calls to
git.refreshwith no arguments, without simulating the state of not yet having performed an initial refresh. This behavior is not changed in this pull request, but it makes sense to test it, doing so may avoid confusion with the different initial-refresh behavior when reading the tests, and the important subtlety of when relative paths are and aren't resolved is made clearer.Test an initial refresh. This is a refresh with no arguments that is performed on import, but to make the relationship to the other tests clearer, reduce test complexity, and make the tests run faster (since some of them run with all documented values of
GIT_PYTHON_REFRESH), this simulates that state by settingGit.GIT_PYTHON_GIT_EXECUTABLEtoNonerather than starting a Python subprocess to do an import.This includes testing the behavioral change made in this PR and described above.
Test the new synonyms of existing values for
GIT_PYTHON_REFRESH(such aslogforwarn).I added the tests first. For the initial-refresh test, I first wrote it for the preexisting behavior, modified it for the desired behavior and saw it fail. Then I applied the changes, causing the tests to pass again. Those changes are based on, but not quite the same as,dc62096 as discussed in#1813 and#1808 (comment).
There is some more information in commit messages, especiallyac50d8e.