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

Get rid of global state in fail and warn #35

Conversation

erlend-aasland
Copy link

@erlend-aasland erlend-aasland commented Dec 23, 2023

Here's an experiment I thought of earlier today, as a step towards error handling without global state. One of the biggest pains wrt. getting rid of the clinic global is that fail depends on it too much. So here's the experiment:

  • tear of the global state from warn_or_fail
  • identify the failing tests, and amend them by supplying the line number and filename explicitly

It is a crude change, but it kind of works, and I think it might be an improvement (if polished a little bit).

Side-effects:

  • some errors now generate more accurate line numbers
  • most warnings (the few we have) generate less accurate line numbers (or no at all)
  • cpp.py now have a cleaner error interface towards clinic (IMO)

IMO, we should just use a variety of custom exceptions, catch these during parsing, format the error message and reraise it (for either the CLI or the test suite to catch).

@erlend-aasland erlend-aasland marked this pull request as ready for review December 28, 2023 15:01
@erlend-aasland
Copy link
Author

After the latest yak-shaving, the diff for this PR is now greatly reduced.

erlend-aasland and others added 2 commits December 28, 2023 16:05
This test must be updated nevertheless when warnings get line numbers.
@erlend-aasland
Copy link
Author

erlend-aasland commented Jan 19, 2024

Victor had some remarks about this PR; I'll try to address them tomorrow:

  • it is unclear what happens if you pass lineno but not filename

@erlend-aasland
Copy link
Author

  • it is unclear what happens if you pass lineno but not filename

Come to think of it, that is also a concern with the current code; this PR does not alter the status quo.

@vstinner
Copy link

I wrote a different approach: python#114752

Maybe some of these changes are still relevent with my change, I didn't dig into the code yet.

@AlexWaygood AlexWaygood removed their request for review January 31, 2024 11:30
@erlend-aasland
Copy link
Author

Upstreamed as python#115510

@erlend-aasland erlend-aasland deleted the clinic/error-revolution branch February 15, 2024 10:52
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