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-113317: Add libclinic.converter and libclinic.crenderdata modules by vstinner · Pull Request #116821 · python/cpython

Conversation

@vstinner

@vstinner vstinner commented Mar 14, 2024 •
edited by bedevere-app Bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member
  • Move CConverter class to a new libclinic.converter module.
  • Move CRenderData and Include classes to a new libclinic.crenderdata module.
  • Issue: Argument Clinic: split out global stateless helpers and constants from clinic.py #113317

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@vstinner
pythongh-113317, AC: Add libclinic.converter module
56e569a 
* Move CConverter class to a new libclinic.converter module.
* Move CRenderData and Include classes to a new libclinic.crenderdata
  module.
@vstinner vstinner added the skip news label Mar 14, 2024
@vstinner
vstinner requested a review from erlend-aasland as a code owner March 14, 2024 16:17
@bedevere-app bedevere-app Bot added the awaiting core review label Mar 14, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Mar 14, 2024
Argument Clinic: split out global stateless helpers and constants from clinic.py #113317
Closed
@vstinner
vstinner merged commit 25cd873 into python:main Mar 14, 2024
@vstinner
vstinner deleted the clinic_converter branch March 14, 2024 17:59
@bedevere-app bedevere-app Bot removed the awaiting core review label Mar 14, 2024
@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
erlend-aasland
erlend-aasland reviewed Mar 19, 2024
View reviewed changes
Comment thread Tools/clinic/libclinic/converter.py
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 = {}

@erlend-aasland erlend-aasland Mar 19, 2024

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

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

@vstinner vstinner Mar 19, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland erlend-aasland Mar 19, 2024

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

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

@vstinner vstinner Mar 19, 2024

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
vstinner added a commit to vstinner/cpython that referenced this pull request Mar 20, 2024
@vstinner
pythongh-113317, AC: Add libclinic.converter module (python#116821)
7059475 
* 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
@vstinner @adorilson
pythongh-113317, AC: Add libclinic.converter module (python#116821)
21498f3 
* 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
@vstinner @diegorusso
pythongh-113317, AC: Add libclinic.converter module (python#116821)
6eb63bc 
* 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

Reviewers

@erlend-aasland erlend-aasland erlend-aasland left review comments

Uh oh!

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

Assignees

No one assigned

Labels

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 @erlend-aasland
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 01402503 — 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.