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-110964: clinic: pass clinic argument to bad_argument() #110984

Closed
wants to merge 1 commit into from

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Oct 17, 2023

Don't rely on the global 'clinic' argument: pass explicitly a 'clinic' argument.

@vstinner
Copy link
Member Author

The only purpose of this change is to avoid using clinic global variable in def bad_argument() method. Just for that, I had to modify many lines :-(

@vstinner
Copy link
Member Author

cc @AlexWaygood

@AlexWaygood
Copy link
Member

AlexWaygood commented Oct 17, 2023

Please give me some time to look at this! :-)

@vstinner vstinner requested review from a team and gpshead as code owners October 17, 2023 22:11
@vstinner
Copy link
Member Author

"Check if generated files are up to date (pull_request)" failure looks unrelated:

./configure --config-cache --with-pydebug --enable-shared
    PKG_CONFIG_PATH: /opt/hostedtoolcache/Python/3.11.6/x64/lib/pkgconfig

configure: loading cache config.cache
configure: error: `PKG_CONFIG_PATH' has changed since the previous run:
configure:   former value:  `/opt/hostedtoolcache/Python/3.11.5/x64/lib/pkgconfig'
configure:   current value: `/opt/hostedtoolcache/Python/3.11.6/x64/lib/pkgconfig'

Don't rely on the global 'clinic' argument: pass explicitly a
'clinic' argument.
@vstinner
Copy link
Member Author

I rebased my PR on the main branch to try to fix the "Check if generated files are up to date" job failure.

@vstinner
Copy link
Member Author

Please give me some time to look at this! :-)

Mmmmh, so do you need more time to review?

@vstinner
Copy link
Member Author

vstinner commented Nov 3, 2023

Well, since @AlexWaygood is not available for review, i think that i will just merge my change next days.

@AlexWaygood
Copy link
Member

AlexWaygood commented Nov 3, 2023

Thanks for waiting, sorry for the slow response from me! I wanted to take the time to think about this properly, but haven't had a chance recently due to being busy at work, and it's unlikely that I'll have the time in the next week either.

My instinct is that there should be a simpler solution here that involves fewer changes, but if you feel like there's a rush to get this merged for whatever reason, then please go ahead. I agree I've kept you waiting for a while :-)

@vstinner
Copy link
Member Author

vstinner commented Nov 3, 2023

me:

The only purpose of this change is to avoid using clinic global variable in def bad_argument() method. Just for that, I had to modify many lines :-(

@AlexWaygood:

My instinct is that there should be q simpler solution here that involves fewer changes,

Yeah, there should be a way to redesign the code to have to pass less arguments. I dislike the current API :-( Moreover, passing limited_capi and clinic is kind of redundant, since using clinic.limited_capi is the same as passing limited_capi.

I close my PR for now. We can revisit this code later when we will try to give rid of the global clinic variable.

@vstinner vstinner closed this Nov 3, 2023
@vstinner vstinner deleted the clinic_bad_arg branch November 3, 2023 11:22
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.

2 participants