Skip to content

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

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
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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants