Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

gh-104683: Argument clinic: cleanupstate_modulename_name()#107340

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
AlexWaygood merged 3 commits intopython:mainfromAlexWaygood:clinic-modulename-name
Jul 27, 2023

Conversation

@AlexWaygood
Copy link
Member

@AlexWaygoodAlexWaygood commentedJul 27, 2023•
edited
Loading

Thestate_modulename_name() function has two variables namedmodule, that have different purposes and that are assigned to objects of different types:

module,cls=self.clinic._module_and_class(fields)

module=None
try:
module=ast.parse(ast_input)
exceptSyntaxError:
pass

If we give the second variable a different name, it makes the code easier to understand for mypy and for humans. I also made some incidental changes to nearby lines to use f-strings rather than string concatenation using+.

(This PR is required in order to add type hints to the last remaining untyped function inclinic.py,_module_and_class().)

@erlend-aasland
Copy link
Contributor

This is a very nice improvement, indeed. A lot of the error paths in this PR don't have test coverage, yet. IMO, we should bring test coverage ofstate_modulename_name() up to 100% before continuing with this PR. I can give that a stab tonight, unless someone beats me to it :)

AlexWaygood reacted with thumbs up emoji

@erlend-aasland
Copy link
Contributor

Thanks!

AlexWaygood reacted with heart emoji

@AlexWaygoodAlexWaygood merged commitc2b1689 intopython:mainJul 27, 2023
@AlexWaygoodAlexWaygood deleted the clinic-modulename-name branchJuly 27, 2023 21:51
@erlend-aasland
Copy link
Contributor

(Sorry, I forgot to mention: we should add tests in separate PRs, since we normally want those backported to bugfix branches.)

@AlexWaygood
Copy link
MemberAuthor

AlexWaygood commentedJul 27, 2023•
edited
Loading

(Sorry, I forgot to mention: we should add tests in separate PRs, since we normally want those backported to bugfix branches.)

In general, I agree -- but here, adding tests revealed some micro-bugs in the failure messages, so I would have had to assert arguably incorrect behaviour in the tests in order to backport them to bugfix branches. Unless we consider the microbugs in the failure messages important enough to warrant backportingthis PR as a bugfix, which I personally don't? Anyway, that was my thought process :)

erlend-aasland reacted with thumbs up emoji

@bedevere-bot

This comment was marked as off-topic.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@erlend-aaslanderlend-aaslanderlend-aasland approved these changes

Assignees

No one assigned

Labels

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@AlexWaygood@erlend-aasland@bedevere-bot
[8]ページ先頭
©2009-2025 Movatter.jp