Have initial refresh use a logger to warn by EliahKagan · Pull Request #1815 · gitpython-developers/GitPython (original) (raw)

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:

WARNING: Bad git executable.

When logging is not configured, logging.lastResort prints that to standard error just like that. However, if logging has been configured, then most commonly a prefix indicating the logger and level is shown. In particular, when logging.basicConfig() is called with no arguments to configure logging, the actual logged message is of course the same, but the way it is displayed begins:

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

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 WARNING should be changed to to Warning so it looks less like a contradictory indication of a logging level? Then again, that may make it slightly less visible when no prefix is shown. I am not sure.

With this PR it seems reasonable to claim that the messaging in GitPython is finally consistent.

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.

Everything uses logging, and messages are no suppressed by default.

I agree. In particular, I looked for other print statements in the git module and did not find them.

Admission: I only skimmed the tests themselves due to my own state of mind and the trust in the PRs author.

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.