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-104683: Argument clinic: pass clinic as a parameter where possible #107435

Merged
merged 1 commit into from Jul 29, 2023

Conversation

AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Jul 29, 2023

This PR started off life with me wanting to get rid of this at the end of Clinic.__init__, which feels like an awful hack to me:

cpython/Tools/clinic/clinic.py

Lines 2206 to 2207 in d0dcd27

global clinic
clinic = self

clinic is elsewhere set to None in the global namespace in two places:

clinic = None

clinic = None

Unfortunately, removing this hack completely is rather difficult, due to the fact that warn_or_fail relies on clinic being a global variable! Passing clinic as a parameter to warn_or_fail would mean that every usage of warn() or fail() would have to be updated, which would have led to a huge diff.

def warn_or_fail(
*args: object,
fail: bool = False,
filename: str | None = None,
line_number: int | None = None,
) -> None:
joined = " ".join([str(a) for a in args])
add, output = text_accumulator()
if fail:
add("Error")
else:
add("Warning")
if clinic:
if filename is None:
filename = clinic.filename
if getattr(clinic, 'block_parser', None) and (line_number is None):
line_number = clinic.block_parser.line_number

Nonetheless, this PR still feels like a (small) improvement over the current state of affairs. In particular, by passing clinic as a parameter to output_templates, we remove the need for an assert clinic is not None assertion. This PR also reduces the changes that will be required in the future, should we want to move output_templates into a separate module...

Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

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

Makes sense. Thanks!

@AlexWaygood AlexWaygood merged commit 5113ed7 into python:main Jul 29, 2023
25 checks passed
@AlexWaygood AlexWaygood deleted the clinic-global-usage branch July 29, 2023 18:47
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.

None yet

3 participants