Skip to content

gh-104050: Argument Clinic: Annotate CLanguage.render_option_group_parsing() - #106929

Merged
erlend-aasland merged 4 commits into
python:mainfrom
erlend-aasland:clinic/annotate-clanguage-render-option-group-parsing
Jul 20, 2023
Merged

gh-104050: Argument Clinic: Annotate CLanguage.render_option_group_parsing()#106929
erlend-aasland merged 4 commits into
python:mainfrom
erlend-aasland:clinic/annotate-clanguage-render-option-group-parsing

Conversation

@erlend-aasland

@erlend-aaslanderlend-aasland commented Jul 20, 2023•
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

@bedevere-botbedevere-bot mentioned this pull request Jul 20, 2023
Closed
AlexWaygood
AlexWaygood reviewed Jul 20, 2023
View reviewed changes

@AlexWaygoodAlexWaygood left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Some unnecessary use of quoted annotations now that we have from __future__ import annotations at the top of the file:

Comment threadTools/clinic/clinic.py Outdated
Comment threadTools/clinic/clinic.py Outdated
AlexWaygood
AlexWaygood reviewed Jul 20, 2023
View reviewed changes
Comment threadTools/clinic/clinic.py
erlend-aasland
erlend-aasland commented Jul 20, 2023
View reviewed changes
Comment threadTools/clinic/clinic.py Outdated
erlend-aasland
erlend-aasland commented Jul 20, 2023
View reviewed changes
Comment threadTools/clinic/clinic.py
erlend-aaslandand others added 2 commits July 20, 2023 22:52
@erlend-aasland@AlexWaygood
Apply suggestions from code review
a13d26b
Extra quotes around Parameter are not needed anymore
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@erlend-aasland
Apply suggestions from code review
4fa647e
Rip out ParamIter
AlexWaygood
AlexWaygood reviewed Jul 20, 2023
View reviewed changes
Comment threadTools/clinic/clinic.py
group = required
else:
right.append(group)
assert group is not None

@AlexWaygoodAlexWaygoodJul 20, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is fine for now, but I feel like there's probably a simpler way of writing the code immediately above that would be easier to understand and not require this assertion here for mypy

@erlend-aaslanderlend-aaslandJul 20, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yes. We should accumulate these smells in the modernising-issue, so we don't forget. Or we can just grep for asserts.

Comment threadTools/clinic/clinic.py Outdated
@erlend-aasland@AlexWaygood
🤦
5fe6c3f
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
AlexWaygood
AlexWaygood approved these changes Jul 20, 2023
View reviewed changes

@AlexWaygoodAlexWaygood left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks!

@erlend-aasland

erlend-aasland commented Jul 20, 2023

Copy link
Copy Markdown
ContributorAuthor

LGTM, thanks!

Thanks, the same!

@erlend-aasland
erlend-aasland enabled auto-merge (squash) July 20, 2023 21:13
@erlend-aasland
erlend-aasland merged commit 42c6300 into python:mainJul 20, 2023
@erlend-aasland
erlend-aasland deleted the clinic/annotate-clanguage-render-option-group-parsing branch July 20, 2023 21:35
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AlexWaygoodAlexWaygoodAlexWaygood approved these changes

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@erlend-aasland@AlexWaygood@bedevere-bot