Skip to content

gh-104683: Modernise Argument Clinic parameter state machine #106362

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

Merged

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Jul 3, 2023

Use enum and pattern matching to make the code more readable.

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.

Great idea; this is a big improvement in my opinion. A few small comments, mostly about idiomatic usage of enums:

@AlexWaygood
Copy link
Member

(I take it you've checked that all this code is well covered by our tests? :)

@erlend-aasland
Copy link
Contributor Author

(I take it you've checked that all this code is well covered by our tests? :)

It's pretty good checked; I ran into test failures bco. some hickups during implementation :) However, I'll do a proper coverage check before landing.

Co-authored-by: Oleg Iarygin <[email protected]>
Co-authored-by: Alex Waygood <[email protected]>
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.

Looks great, providing the test coverage is good. I left a comment about a possible enhancement you could do with the enum, but I think it's probably a case of YAGNI.

@erlend-aasland erlend-aasland enabled auto-merge (squash) July 3, 2023 20:48
@erlend-aasland erlend-aasland merged commit 71b4044 into python:main Jul 3, 2023
@erlend-aasland erlend-aasland deleted the clinic/param-state-machine branch July 3, 2023 21:52
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.

4 participants