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

Refactor Argument Clinic support code into libclinic #113299

Closed
vstinner opened this issue Dec 20, 2023 · 12 comments
Closed

Refactor Argument Clinic support code into libclinic #113299

vstinner opened this issue Dec 20, 2023 · 12 comments
Labels
topic-argument-clinic type-feature A feature request or enhancement

Comments

@vstinner
Copy link
Member

vstinner commented Dec 20, 2023

Argument Clinic was added 10 years ago. The script clinic.py grown from around 2500 lines to around 6700 lines.

I propose to split this large file into sub-files to ease the maintenance of Argument Clinic.

Linked PRs

vstinner added a commit to vstinner/cpython that referenced this issue Dec 20, 2023
* Create Tools/clinic/clinic/ package
* Add Tools/clinic/clinic/__init__.py: export names, most of them are
  used by tests.
* Use vars(module) instead of globals() to get symbols.
* Move header (author, license) to clinic/__init__.py.
* Add run_clinic.py script in Tools/clinic/. Update "make clinic"
  and "make clinic-tests" in Makefile.pre.in to use run_clinic.py.
@erlend-aasland
Copy link
Contributor

This has also been discussed before. See #104683 (comment).

@erlend-aasland
Copy link
Contributor

erlend-aasland commented Dec 20, 2023

I think it is a really good idea to split clinic.py into multiple files. The C preprocessor stuff is already separated out (cpp.py). We should do this in iterations; not everything in one big PR. Perhaps it is easiest to start with the converters; it's a large chunk of clinic.py, and I think it would be pretty easy to separate it from the rest. Separating out the application is also a good start.

Moreover, doing this work in chunks/iterations, instead of everything in one go, will minimise the impact on ongoing Argument Clinic work.

@AlexWaygood
Copy link
Member

+1 to splitting clinic.py up, and +1 to doing it incrementally rather than in one big PR.

Previously I've held off from doing something like this because clinic.py's use of global variables means that lots of parts of the file are tightly coupled to other parts in surprising and unfortunate ways. In #113300, warn() and fail() still use a global clinic variable in utils.py, but that variable is now altered from code in other files, making it even harder to understand what's going on. That feels worse, and it feels like we're avoiding solving the actual problem here, which is that clinic.py just shouldn't really be using global variables like this in the first place.

But maybe it's acceptable for now, if splitting the code up into several files is a higher priority :) Getting rid of the global variables is hard!

@erlend-aasland
Copy link
Contributor

IMO, getting rid of the global variables should be a prerequisite to splitting things up. We don't need to rush this; clinic.py has been a single file since it started out in life. Argument Clinic is already hard to comprehend, so any refactoring, including splitting up should make it more readable, not the other way around. Readability is IMO the highest priority.

vstinner added a commit to vstinner/cpython that referenced this issue Dec 20, 2023
* Create Tools/clinic/libclinic/ package.
* Move cpp.py to libclinic.
* Create libclinic.utils.
@vstinner

This comment was marked as off-topic.

@AlexWaygood

This comment was marked as off-topic.

@erlend-aasland

This comment was marked as off-topic.

@erlend-aasland

This comment was marked as off-topic.

@vstinner

This comment was marked as resolved.

@erlend-aasland erlend-aasland closed this as not planned Won't fix, can't repro, duplicate, stale Dec 20, 2023
@erlend-aasland erlend-aasland changed the title Argument Clinic: split clinic.py into sub-files Refactor Argument Clinic support code into libclinic Dec 27, 2023
@erlend-aasland
Copy link
Contributor

Since last week, we've done a lot of yak-shaving (and there is more to be done). A bare bones libclinic has been established, containing most of the stateless formatting code. The next steps will be to move ClinicError into libclinic/errors.py, and rename cpp.py to libclinic/cpp.py.

@erlend-aasland
Copy link
Contributor

libclinic/errors.py was established via #113525.

@erlend-aasland
Copy link
Contributor

Superseded by #113317

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic-argument-clinic type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

3 participants