◐ Shell
reader mode source ↗
Skip to content

gh-108494: AC: change parse_file() API#108504

Closed
vstinner wants to merge 2 commits into
python:mainfrom
vstinner:ac_args
Closed

gh-108494: AC: change parse_file() API#108504
vstinner wants to merge 2 commits into
python:mainfrom
vstinner:ac_args

Conversation

@vstinner

@vstinner vstinner commented Aug 26, 2023 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

Revert my change adding 'ns' parameter, add back 'verify' parameter, and add also 'limited_capi' parameter.

@bedevere-bot bedevere-bot mentioned this pull request Aug 26, 2023
Closed
@vstinner vstinner mentioned this pull request Aug 26, 2023
Merged
@vstinner

vstinner commented Aug 27, 2023

Copy link
Copy Markdown
Member Author

I'm waiting for the review @AlexWaygood, he proposed that change.

@AlexWaygood

AlexWaygood commented Aug 27, 2023

Copy link
Copy Markdown
Member

Thanks for fixing this up!

I had a work event all day yesterday and I'm visiting family today; I'd like to take a proper look tomorrow

@vstinner
pythongh-108494: AC: change parse_file() API …
0b1ae97 
Revert my change adding 'ns' parameter, add back 'verify' parameter,
and add also 'limited_capi' parameter.
@vstinner
Clinic constructor uses DEFAULT_LIMITED_CAPI
e9f70f2
@vstinner

vstinner commented Aug 28, 2023

Copy link
Copy Markdown
Member Author

@AlexWaygood: Would you mind to review the updated PR? Do you prefer to keep DEFAULT_LIMITED_CAPI, or remove it?

@AlexWaygood

AlexWaygood commented Aug 28, 2023

Copy link
Copy Markdown
Member

I still have a bunch of nits: I don't think we need the MockClinic class that was added to the test file in 1dd9510; the types import in the test file is now an unused import; and I still think it would be better to make limited_capi a required argument everywhere. I made #108575 as an alternative PR, which builds on this one :)

@AlexWaygood AlexWaygood mentioned this pull request Aug 28, 2023
Merged
@vstinner vstinner deleted the ac_args branch August 28, 2023 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AlexWaygood AlexWaygood

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

@erlend-aasland erlend-aasland erlend-aasland approved these changes

Assignees

No one assigned

Labels

awaiting merge skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@vstinner @AlexWaygood @serhiy-storchaka @erlend-aasland @bedevere-bot