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

Set default logging level to WARNING #1178

Merged
merged 5 commits into from
Jan 1, 2024
Merged

Conversation

dukecat0
Copy link
Member

  • I have added an entry to docs/changelog.md

Summary of changes

Set the default logging level to WARNING, so debug log messages won't be shown without passing additional flags such as --verbose.

Closes #1175
Closes #1177

Test plan

Tested by running

nox -s tests-3.12

src/pipx/main.py Outdated Show resolved Hide resolved
@dukecat0
Copy link
Member Author

dukecat0 commented Jan 1, 2024

Probably we need to update this line to ERROR and CRITICAL logging levels.?

" WARNING, ERROR, and CRITICAL logging levels."

@chrysle
Copy link
Contributor

chrysle commented Jan 1, 2024

Yes, looks like it.

@mayeut
Copy link
Member

mayeut commented Jan 1, 2024

The "-q" added in #1159 shall also be removed:

assert not run_pipx_cli(["list", "-q", "--json"])

@dukecat0
Copy link
Member Author

dukecat0 commented Jan 1, 2024

We can change its behavior instead of removing it. I remember the original usage of this flag was hiding all console outputs, not just the debug log messages.

@dukecat0 dukecat0 merged commit 6d47060 into pypa:main Jan 1, 2024
14 checks passed
@dukecat0 dukecat0 deleted the fix-logging-lvl branch January 1, 2024 13:23
@mayeut
Copy link
Member

mayeut commented Jan 1, 2024

We can change its behavior instead of removing it. I remember the original usage of this flag was hiding all console outputs, not just the debug log messages.

It might be so but:
IMHO, it should have raised a red flag in review (but can be easily missed).
Removing the flag in that test allows to see that the default log level is the correct one (for now, it seems it's the only test for that - although probably accidental).
Given this PR reverts the default log level to WARNING, I think we should also revert this change that was only introduced because of the default log level change.

If there are more changes to be made as you suggest, then it would be for another PR but for now, I think changing back this test is the best option.

@dukecat0
Copy link
Member Author

dukecat0 commented Jan 1, 2024

I've opened #1181 to revert it now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants