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

gh-104683: Rework Argument Clinic error handling #107551

Merged
merged 31 commits into from
Aug 3, 2023

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Aug 1, 2023

Introduce ClinicError and ClinicWarning:

  • fail() raises ClinicError
  • warn() uses warnings.warn(msg, ClinicWarning)

In main(), catch ClinicError, format the error message,
print to stderr, and exit with an error. Adapt the test
suite accordingly.

erlend-aasland and others added 10 commits July 30, 2023 00:33
- Introduce ClinicError and ClinicWarning
- fail() raises ClinicError
- warn() uses warnings.warn(msg, ClinicWarning)
- the CLI runs main(), catches ClinicError, formats the error message
  prints to stderr and exits with an error
- adapt the test suite to work with ClinicError
Tools/clinic/clinic.py Outdated Show resolved Hide resolved
Lib/test/test_clinic.py Outdated Show resolved Hide resolved
Tools/clinic/clinic.py Outdated Show resolved Hide resolved
Tools/clinic/clinic.py Outdated Show resolved Hide resolved
Lib/test/test_clinic.py Outdated Show resolved Hide resolved
Lib/test/test_clinic.py Outdated Show resolved Hide resolved
Lib/test/test_clinic.py Outdated Show resolved Hide resolved
Lib/test/test_clinic.py Outdated Show resolved Hide resolved
@erlend-aasland erlend-aasland marked this pull request as ready for review August 2, 2023 14:58
@erlend-aasland
Copy link
Contributor Author

Ready for a new round of review! Finally, all (or most) line number are now correct. If I introduce an error in Lib/test/clinic.test.c, I now get an accurate line number. Yay! I'm not sure why some of the tests are off by one, though. Perhaps it has to do with FakeClinic.

Comment on lines 4612 to 4615
try:
self.state(line)
except ClinicError as exc:
raise ClinicError(str(exc), filename=exc.filename, lineno=line_number)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, this is a workaround! We should ensure that line numbers are correctly updated in the state machine. Let's create an issue about this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to keep the workaround for now; it really helps to have accurate line numbers when debugging.

Tools/clinic/clinic.py Outdated Show resolved Hide resolved
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@AlexWaygood
Copy link
Member

I had an idea for how to deduplicate some code between warn_and_fail() and the main() function -- made a PR to your branch at erlend-aasland#49

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last suggestion to help future maintainers -- looks great to me otherwise!

Lib/test/test_clinic.py Show resolved Hide resolved
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! :D

@erlend-aasland erlend-aasland enabled auto-merge (squash) August 2, 2023 23:39
@erlend-aasland
Copy link
Contributor Author

Great, thank you; this PR is in a lot better shape now 😄

@erlend-aasland erlend-aasland merged commit 1cd479c into python:main Aug 3, 2023
16 checks passed
@erlend-aasland erlend-aasland deleted the clinic/warn-and-fail branch August 3, 2023 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants