GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-110964: clinic: pass clinic argument to bad_argument() by vstinner · Pull Request #110984 · python/cpython

Conversation

@vstinner

@vstinner vstinner commented Oct 17, 2023 •
edited by bedevere-app Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

  • Issue: [C API] Move undocumented private _PyArg C API to pycore_modsupport.h internal C API #110964

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@vstinner vstinner added the skip news label Oct 17, 2023
@vstinner
vstinner requested review from AlexWaygood and erlend-aasland as code owners October 17, 2023 12:50
@bedevere-app bedevere-app Bot mentioned this pull request Oct 17, 2023
[C API] Move undocumented private _PyArg C API to pycore_modsupport.h internal C API #110964
Closed
@bedevere-app bedevere-app Bot added the awaiting core review label Oct 17, 2023
@vstinner

vstinner commented Oct 17, 2023

Copy link
Copy Markdown
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 :-(

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Oct 17, 2023

Copy link
Copy Markdown
Member Author

cc @AlexWaygood

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner vstinner mentioned this pull request Oct 17, 2023
gh-110964: Remove private _PyArg functions #110966
Merged
@AlexWaygood

AlexWaygood commented Oct 17, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

vstinner commented Oct 17, 2023

Copy link
Copy Markdown
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'
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner
pythongh-110964: clinic: pass clinic argument to bad_argument()
26db3ea
Don't rely on the global 'clinic' argument: pass explicitly a
'clinic' argument.
@vstinner
vstinner force-pushed the clinic_bad_arg branch from b62fdc2 to 26db3ea Compare October 18, 2023 15:46
@vstinner

vstinner commented Oct 18, 2023

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Oct 28, 2023

Copy link
Copy Markdown
Member Author

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

Mmmmh, so do you need more time to review?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Nov 3, 2023

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@AlexWaygood

AlexWaygood commented Nov 3, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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 :-)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Nov 3, 2023

Copy link
Copy Markdown
Member Author

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.

AlexWaygood and erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner vstinner closed this Nov 3, 2023
@vstinner
vstinner deleted the clinic_bad_arg branch November 3, 2023 11:22
@vstinner vstinner mentioned this pull request Dec 20, 2023
gh-113299: Create libclinic package #113309
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@erlend-aasland erlend-aasland Awaiting requested review from erlend-aasland erlend-aasland is a code owner
@AlexWaygood AlexWaygood Awaiting requested review from AlexWaygood
@gpshead gpshead Awaiting requested review from gpshead gpshead is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting core review skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@vstinner @AlexWaygood
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.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.