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

[DRAFT] gh-113299: Split clinic.py into sub-files #113300

Closed
wants to merge 2 commits into from

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented 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.

* 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.
@vstinner
Copy link
Member Author

Current files sorted by their number of lines:

$ (cd Tools/clinic/clinic/; wc -l *.py|sort -n)
    32 __init__.py
    58 crender_data.py
    82 parameter.py
   124 language.py
   164 cli.py
   179 function.py
   194 cpp.py
   198 utils.py
   204 return_converter.py
   212 block_printer.py
   250 block_parser.py
   374 clinic.py
   578 converter.py
  1192 converters.py
  1667 clanguage.py
  1667 parser.py
  7175 total

There are 4 files with at least 500 lines:

  • parser.py (1667 lines)
  • clanguage.py (1667 lines)
  • converters.py (1192 lines
  • converter.py (578 lines)

Maybe small files (under 200 lines) should be merged into larger files?

  • crender_data.py
  • parameter.py
  • language.py
  • cli.py
  • function.py

@vstinner
Copy link
Member Author

vstinner commented Dec 20, 2023

This PR is a draft to discuss the idea of splitting clinic.py into sub-files and if we want to split it, decide how to split it.

I didn't try to fix mypy, I don't think that it's worth it at this point. clinic.py was modified since I started to work on the file, so there are already conflict. I will only consider fixing conflicts once we reach a consensus.

If we split clinic.py, backporting changes will be more tricky. I don't know if it's common to have to backport Argument Clinic bugfixes to stable branches. I only paid attention to new features, and new features should only be added to the main branch.

cc @AlexWaygood @erlend-aasland @corona10 @serhiy-storchaka

@vstinner
Copy link
Member Author

TODO:

  • Review code using vars() to get module globals, especially code creating a dict from multiple modules. Maybe such dict should only be created once? Or an abstraction should be added?
  • Review using of the new global variable: clinic.clinic.clinic (main) and clinic.utils.clinic (copy used by warn_of_fail() to get clinic.filename and clinic.block_parser.line_number).
  • Review __init__.py design: currently, it's mostly written for test_clinic.py.
  • Review imports.
  • Fix mypy.
  • Fix merge conflicts.

@corona10
Copy link
Member

As I discussed: before #113160 will be landed soon. You may also need to reflect the change once the PR is merged.

@corona10
Copy link
Member

corona10 commented Dec 20, 2023

Can we remain the name of run_clinic.py as the just clinic.py to maintain commit logs as possible?
Even if git can track the deleted file, it's not the same developing experience that can be easily tracked from GitHub directly.

@vstinner
Copy link
Member Author

Tools/clinic/clinic/clinic.py repeats clinic 3 times... Maybe Tools/clinic/ should be the package, and run_clinic.py should be moved to Tools/build/.

@corona10
Copy link
Member

We can just use 1-depth policy like case_generator, why do we have to use 2-depths?
https://github.com/python/cpython/tree/main/Tools/cases_generator

@vstinner
Copy link
Member Author

As I discussed: before #113160 will be landed soon. You may also need to reflect the change once the PR is merged.

As written, I will handle merge conflicts later. This PR is a draft to discuss the design of such split.

Even if git can track the deleted file, it's not the same developing experience that can be easily tracked from GitHub directly.

Git doesn't store renamed files. It only stores "add" and "remove" operations. Most Git commands (such as git blame and git log) use an heuristic to detect file renames. Example of my ~/.gitconfig:

[diff]
# Disable the limit of rename, because my computers have a lot of memory!
# The default limit is 1000 files. It's not enough for complex cherry-pick on
# openstack-nova for example.
renamelimit=0

[merge]
renameLimit = 0

Can we remain the name of run_clinic.py as the just clinic.py to maintain commit logs as possible?

I don't think that it works well to have a clinic.py script and a clinic package in the same directory. I'm not sure of the benefits in terms of Git history if we apply your proposed idea. I didn't look at Git history.

@corona10
Copy link
Member

corona10 commented Dec 20, 2023

About git history:
I am not saying new files should be able to track old logs, but if you maintain the Tools/clinic/clinic.py and split it, then we can track history before your refactoring PR directly from GitHub website or git log.

@erlend-aasland
Copy link
Contributor

Let's discuss the strategy on the issue and implementation details for a specific PR on that specific PR.

@vstinner
Copy link
Member Author

@erlend-aasland:

Let's discuss the strategy on the issue and implementation details for a specific PR on that specific PR.

I created a concrete draft PR to see how the code can look like once the big clinic.py is splitted into sub-files. I didn't intent to merge it as it is. I close the PR to make it more obvious :-)

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.

3 participants