Skip to content

gh-113317: Add ParseArgsCodeGen class #117707

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Apr 11, 2024
Merged

gh-113317: Add ParseArgsCodeGen class #117707

merged 4 commits into from
Apr 11, 2024

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Apr 10, 2024 •
edited by bedevere-app bot
Loading

@bedevere-app bedevere-app bot mentioned this pull request Apr 10, 2024
Closed
@vstinner
Copy link
Member Author

This PR is based on PR gh-117626.

@vstinner vstinner force-pushed the clinic_parse_args branch from 781133c to b17f552 Compare April 11, 2024 10:16
@vstinner
Copy link
Member Author

PR rebased on top of merged #117626.

@vstinner vstinner marked this pull request as ready for review April 11, 2024 10:17
@erlend-aasland erlend-aasland changed the title [WIP] gh-113317: Add ParseArgsCodeGen class gh-113317: Add ParseArgsCodeGen class Apr 11, 2024
@erlend-aasland
Copy link
Contributor

The previous PR used the Codegen spelling. This PR uses CodeGen. Consider using only one variant :)

@erlend-aasland
Copy link
Contributor

This is nice!

Should we consider adding this as a separate module? Or would libclinic.codegen be a better home for it?

@vstinner
Copy link
Member Author

Should we consider adding this as a separate module?

CLanguage uses ParseArgsCodeGen which uses CLanguage... There is an inter-dependency. It's more convenient to have both classes in the same file.

@erlend-aasland
Copy link
Contributor

Yes, there are inter-dependencies everywhere :(

@vstinner
Copy link
Member Author

The previous PR used the Codegen spelling. This PR uses CodeGen. Consider using only one variant :)

I updated this PR to rename Codegen to CodeGen.

@vstinner
Copy link
Member Author

Should we consider adding this as a separate module?

I broke the inter-dependency between CLanguage and ParseArgsCodeGen, and I moved declare_parser() and ParseArgsCodeGen to a new libclinic.parse_args module.

@erlend-aasland
Copy link
Contributor

I broke the inter-dependency between CLanguage and ParseArgsCodeGen, and I moved declare_parser() and ParseArgsCodeGen to a new libclinic.parse_args module.

Nice, so now we can make the parser templates globals of the libclinic.parse_args module, instead of class members.

@vstinner
Copy link
Member Author

Nice, so now we can make the parser templates globals of the libclinic.parse_args module, instead of class members.

Done.

@vstinner vstinner enabled auto-merge (squash) April 11, 2024 13:48
@vstinner vstinner merged commit 671cb22 into python:main Apr 11, 2024
40 checks passed
@vstinner vstinner deleted the clinic_parse_args branch April 11, 2024 13:49
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
@vstinner @diegorusso
pythongh-113317: Add ParseArgsCodeGen class (python#117707)
8218ad9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

Assignees
No one assigned
Labels
skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@vstinner @erlend-aasland