8000 PoC: Use logging for warnings by erlend-aasland · Pull Request #36 · Argument-Clinic/cpython · GitHub
[go: up one dir, main page]
Skip to content

PoC: Use logging for warnings #36

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

PoC: Use logging for warnings #36

wants to merge 4 commits into from

Conversation

erlend-aasland
Copy link

Another experiment: tear out the custom warning functionality, and instead use logging.warning.

Pro: simplified error handling code
Con: no line numbers for warnings1

Footnotes

  1. well, there were never usable line numbers for warnings anyway ↩

@erlend-aasland
Copy link
Author

Could be a nice yak-shave for #35

@erlend-aasland
Copy link
Author
erlend-aasland commented Jan 3, 2024 •
edited
Loading

Another nice side effect is that post this, we could easily sprinkle Argument Clinic with debug and info log messages; it could significantly help improve the Argument Clinic debug experience.

@erlend-aasland
Copy link
Author

What do you think, should we upstream this, @AlexWaygood? After this, it should be trivial to get rid of the global clinic object in the remaining fail().

@vstinner
Copy link

After this, it should be trivial to get rid of the global clinic object in the remaining fail().

Can you explain how this change make it easier to remove the global clinic object in fail()?

@erlend-aasland
Copy link
Author

Can you explain how this change make it easier to remove the global clinic object in fail()?

warn() and fail() currently share implementation in warn_or_fail; tearing them out one at the time makes for better targeted PRs that are easier to review, and have smaller diffs.

@vstinner
Copy link

But how do you avoid access clinic global variable thanks to the logging module? You still need to retrieve filename and line number location somehow, no?

@erlend-aasland
Copy link
Author

But how do you avoid access clinic global variable thanks to the logging module? You still need to retrieve filename and line number location somehow, no?

We never had line numbers for warn(); they are called from places in clinic.py where we don't have the line number at hand. IMO, it is better to acknowledge that fact, and just produce the warning. We have the filename, as you see from the PR :)

@vstinner vstinner mentioned this pull request Jan 30, 2024
Closed
@vstinner
Copy link

FYI I wrote python#114752 to remove the usage global variable clinic.

@AlexWaygood AlexWaygood removed their request for review January 31, 2024 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@erlend-aasland @vstinner
0