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-113317: AC converter: Use add_include() in bad_argument() #114324

Closed
wants to merge 1 commit into from

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Jan 19, 2024

Rename also the 'clinic' global variable to 'global_clinic'.

@vstinner
Copy link
Member Author

Ah, my simple CConverter.add_include() implementation reached its limits:

  File "/home/runner/work/cpython/cpython/./Tools/clinic/clinic.py", line 3266, in bad_argument
    self.add_include('pycore_modsupport.h', '_PyArg_BadArgument()')
  File "/home/runner/work/cpython/cpython/./Tools/clinic/clinic.py", line 3339, in add_include
    raise ValueError("a converter only supports a single include")
ValueError: a converter only supports a single include

@erlend-aasland
Copy link
Contributor

Ah, my simple CConverter.add_include() implementation reached its limits:

  File "/home/runner/work/cpython/cpython/./Tools/clinic/clinic.py", line 3266, in bad_argument
    self.add_include('pycore_modsupport.h', '_PyArg_BadArgument()')
  File "/home/runner/work/cpython/cpython/./Tools/clinic/clinic.py", line 3339, in add_include
    raise ValueError("a converter only supports a single include")
ValueError: a converter only supports a single include

Yeah, I suspected that :) Well, it should be straight forward to support multiple includes in converters; we could extract the nice "condition" logic you added for Clinic.add_include and reuse that for CConverter.add_include.

@erlend-aasland
Copy link
Contributor

Also, I think the clinic global renaming should be kept out; this PR is likely to grow a little bit.

* Rename the 'clinic' global variable to 'global_clinic'.
* Make CConverter.add_include() smarter. Don't fail when adding
  exactly the same include twice.
* Copy converters includes later in output_templates().
@vstinner
Copy link
Member Author

vstinner commented Jan 19, 2024

Sorry, I should have run tests locally, or mark this PR as a draft. I didn't expect so many back and forth.

All issues should now be fixed (let's see what the CI says).

# added late
for converter in converters:
include = converter.include
if include is not None:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that can be made an assert.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not all converters need a special include. Do you mean that include variable is never None?

@vstinner
Copy link
Member Author

commit e14930f was merged instead, I close my PR.

@vstinner vstinner closed this Jan 30, 2024
@vstinner vstinner deleted the ac_add_include branch January 30, 2024 15:20
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