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

PoC: Get rid of some globals #30

Closed

Conversation

erlend-aasland
Copy link

@erlend-aasland erlend-aasland commented Dec 20, 2023

Draft PR for an initial libclinic. The commits are atomic, so we can easily split this up in multiple PRs, if wanted.


📚 Documentation preview 📚: https://cpython-previews--30.org.readthedocs.build/

@erlend-aasland erlend-aasland force-pushed the clinic/globals branch 2 times, most recently from e64f7e3 to e12a8b1 Compare December 20, 2023 23:02

Choose a reason for hiding this comment

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

Possibly even test_clinic could become a package, and tests for helpers could be put in submodules? But we can also do that later, of course

Copy link
Author

Choose a reason for hiding this comment

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

Yes, that's a good idea for a follow-up enhancement.


__all__ = [
# Accumulators
"_TextAccumulator",

Choose a reason for hiding this comment

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

It feels a bit weird to include things with leading underscores in __all__ and import them from elsewhere. IIRC we've remarked before on the text-accumulator API not having great names. We should probably tackle that chunk of clinic.py in its own PR, and try to think through which bits of the text-accumulator stuff really need to be public-API (and what they should be renamed to, if they currently have private names)

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, this naming is very irritating. Let's fix it first.

Copy link
Author

Choose a reason for hiding this comment

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

I wonder if we really need the underscore version; we can just do _, out, add = text_accumulator instead. Also, we talked about just using lists and join, and just get rid of that whole API.

Copy link
Author

Choose a reason for hiding this comment

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

I did some experiments and landed on something that might work. Take a look at #31.

Copy link
Author

Choose a reason for hiding this comment

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

Also: #33

@AlexWaygood
Copy link

I'll take a deeper look tomorrow!

Copy link

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

This is great. There'll obviously be merge conflicts after python#113402 is merged, and I left one or two minor nitpicks, but other than that -- let's do it!

Tools/clinic/clinic.py Outdated Show resolved Hide resolved
Tools/clinic/clinic.py Show resolved Hide resolved
Tools/clinic/libclinic/__init__.py Outdated Show resolved Hide resolved
Split up clinic.py by establishing libclinic as a support package for
Argument Clinic. Get rid of clinic.py globals by either making them
class members, or by putting them into libclinic.

- Move INCLUDE_COMMENT_COLUMN to BlockPrinter
- Move NO_VARARG to CLanguage
- Move formatting helpers to libclinic
- Move some constants to libclinic (and annotate them as Final)
@erlend-aasland
Copy link
Author

Moved upstream (I'll address your latest comments there):

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants