-
-
Notifications
You must be signed in to change notification settings - Fork 31.1k
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-108494: Argument clinic: Improve the parse_file()
API
#108575
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Revert my change adding 'ns' parameter, add back 'verify' parameter, and add also 'limited_capi' parameter.
Revert my change adding 'ns' parameter, add back 'verify' parameter, and add also 'limited_capi' parameter.
erlend-aasland
approved these changes
Aug 28, 2023
vstinner
approved these changes
Aug 28, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, whatever works :-) I'm not going to nitpick on the coding style ;-)
Thanks for the reviews! |
This comment was marked as spam.
This comment was marked as spam.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ns
argument toparse_file()
; restore theverify
argument that was removed in 1dd9510.limited_capi
argument toparse_file()
. Make thelimited_capi
argument to bothClinic.__init__
andparse_file
required, so that callers always have to be explicit about whether the limited C API is desired or not. This means that there is a "single source of truth" about what the default is, rather than this being duplicated between the global constant and the logic in thecreate_cli()
function.MockClinic
from the test file, which was added in 1dd9510: just use our existing_make_clinic()
helper function instead.With this PR, we would need to make only these changes if we wanted to change it tomorrow so that the limited C API is the default for AC-generated code (plus a few fixes to some tests that currently depend on being able to use our internal C API):
Closes #108504