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

Rework error handling #2

Closed
wants to merge 4 commits into from
Closed

Rework error handling #2

wants to merge 4 commits into from

Conversation

erlend-aasland
Copy link

  • 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

- 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
@erlend-aasland
Copy link
Author

@AlexWaygood, what do you think of this approach?

@erlend-aasland
Copy link
Author

Can be broken up like this:

  • PR 1: split out create_cli()
  • PR 2: add and use ClinicWarning
  • PR 3: add and use ClinicError (including test adjustments)

@erlend-aasland
Copy link
Author

(BTW, since I touched pretty much every test that checks failure of some kind, I just normalised variable naming for them all)

Copy link

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

Looks great overall. A few comments below, but nothing major:

Lib/test/test_clinic.py Show resolved Hide resolved
Lib/test/test_clinic.py Show resolved Hide resolved
Tools/clinic/clinic.py Show resolved Hide resolved
Tools/clinic/clinic.py Show resolved Hide resolved
Tools/clinic/clinic.py Outdated Show resolved Hide resolved
Tools/clinic/clinic.py Outdated Show resolved Hide resolved
@AlexWaygood
Copy link

I have a few more thoughts after you've taken a look at my first round of review ;)

But again, nothing major!

@erlend-aasland
Copy link
Author

Since you agree with the overall approach, let's take this to the cpython repo in smaller steps :)

@AlexWaygood
Copy link

AlexWaygood commented Jul 30, 2023

Yep. One more thing I would say is that it would be nice to restructure the top-level if __name__ == "__main__" block like this (renaming your existing main() function in this PR to be a run_clinic() function):

def main(argv: list[str] | None = None) -> None:
    cli = create_cli()
    try:
        args = cli.parse_args(argv)
        run_clinic(args)
        sys.exit(0)
    except CLIError as exc:
        if msg := str(exc):
            sys.stderr.write(f"Usage error: {msg}\n")
        cli.print_usage()
        sys.exit(1)
    except ClinicError as exc:
        msg = textwrap.dedent(f"""\
            Error in file {exc.filename!r} on line {exc.lineno}:
            {exc}
        """)
        sys.stderr.write(str(exc))
        sys.exit(1)

if __name__ == "__main__":
    main()

(ArgumentParser.parse_args() takes an optional parameter -- if a list of strings is provided to that parameter, it will parse those args instead of sys.argv.)

If we did that, it would be very easy to do end-to-end tests of the CLI without using subprocesses. (The tests got a fair bit slower for me locally on Windows since we started using subprocesses -- they have a lot of overhead on Windows, sadly 😞). When the CLI is actually run, cli.parse_args() will use sys.argv, but in tests, we can pass arbitrary arguments to the main() function to accurately mock a real run of the CLI.

@erlend-aasland
Copy link
Author

If we did that, it would be very easy to do end-to-end tests of the CLI without using subprocesses. (The tests got a fair bit slower for me locally on Windows since we started using subprocesses -- they have a lot of overhead on Windows, sadly 😞). When the CLI is actually run, cli.parse_args() will use sys.argv, but in tests, we can pass arbitrary arguments to the main() function to accurately mock a real run of the CLI.

That's a very good idea. Reducing CI (and local dev workflow) time is a very good thing.

@erlend-aasland
Copy link
Author

python#107551

@erlend-aasland erlend-aasland deleted the clinic/warn-and-fail branch August 1, 2023 23:22
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