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: Add libclinic.converter and libclinic.crenderdata modules #116821

Merged
merged 1 commit into from
Mar 14, 2024

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Mar 14, 2024

  • Move CConverter class to a new libclinic.converter module.
  • Move CRenderData and Include classes to a new libclinic.crenderdata module.

* Move CConverter class to a new libclinic.converter module.
* Move CRenderData and Include classes to a new libclinic.crenderdata
  module.
@vstinner vstinner merged commit 25cd873 into python:main Mar 14, 2024
38 checks passed
@vstinner vstinner deleted the clinic_converter branch March 14, 2024 17:59
@erlend-aasland erlend-aasland changed the title gh-113317, AC: Add libclinic.converter module gh-113317: Add libclinic.converter and libclinic.crenderdata modules Mar 19, 2024
Comment on lines +523 to +534
# maps strings to callables.
# these callables must be of the form:
# def foo(name, default, *, ...)
# The callable may have any number of keyword-only parameters.
# The callable must return a CConverter object.
# The callable should not call builtins.print.
converters: ConverterDict = {}

# maps strings to callables.
# these callables follow the same rules as those for "converters" above.
# note however that they will never be called with keyword-only parameters.
legacy_converters: ConverterDict = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

These belong to the Argument Clinic app (or CLI if you will), so IMO they should not be part of the Argument Clinic library. IMO, it is part of the Argument Clinic app config.

Copy link
Member Author

@vstinner vstinner Mar 19, 2024

Choose a reason for hiding this comment

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

I would prefer to move as much code as possible inside libclinic, including the CLI and "app" logic. Apparently, we have a disagreement on that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I do not oppose to move the CLI / app into libclinic. What I'm saying is I think the global converters and legacy_converters dicts should be a part of the CLI config.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, that makes sense. The practical issue is to reorganize code (move code around) when they are inter-dependenies and most code is left in clinic.py. Code from libclinic/ cannot get code from clinic.py. Once more code will be moved to libclinic, it will be easier to group related code.

vstinner added a commit to vstinner/cpython that referenced this pull request Mar 20, 2024
* Move CConverter class to a new libclinic.converter module.
* Move CRenderData and Include classes to a new libclinic.crenderdata
  module.
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 25, 2024
* Move CConverter class to a new libclinic.converter module.
* Move CRenderData and Include classes to a new libclinic.crenderdata
  module.
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
* Move CConverter class to a new libclinic.converter module.
* Move CRenderData and Include classes to a new libclinic.crenderdata
  module.
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