Skip to content

gh-104504: Run mypy on cases_generator in CI (and blacken the code) - #108090

Merged
corona10 merged 17 commits into
python:mainfrom
corona10:gh-104504
Aug 18, 2023
Merged

gh-104504: Run mypy on cases_generator in CI (and blacken the code)#108090
corona10 merged 17 commits into
python:mainfrom
corona10:gh-104504

Conversation

@corona10

@corona10corona10 commented Aug 17, 2023•
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

@bedevere-botbedevere-bot mentioned this pull request Aug 17, 2023
Closed
@corona10
corona10 marked this pull request as ready for review August 17, 2023 16:55
@corona10

corona10 commented Aug 17, 2023•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

I am not typing expert, so there might be a more promising way to attach type hints for this.

corona10
corona10 commented Aug 17, 2023
View reviewed changes
Comment threadTools/cases_generator/generate_cases.py
corona10
corona10 commented Aug 17, 2023
View reviewed changes
Comment threadTools/cases_generator/analysis.py
@corona10

corona10 commented Aug 17, 2023

Copy link
Copy Markdown
MemberAuthor

cc @sobolevn

AlexWaygood
AlexWaygood reviewed Aug 17, 2023
View reviewed changes
Comment threadTools/cases_generator/requirements-dev.txt Outdated
AlexWaygood
AlexWaygood reviewed Aug 17, 2023
View reviewed changes
Comment threadTools/cases_generator/mypy.ini Outdated
AlexWaygood
AlexWaygood reviewed Aug 17, 2023
View reviewed changes
Comment threadTools/cases_generator/requirements-dev.txt Outdated
AlexWaygood
AlexWaygood reviewed Aug 17, 2023
View reviewed changes
Comment threadTools/cases_generator/formatting.py Outdated
Comment threadTools/cases_generator/generate_cases.py Outdated
hugovk
hugovk reviewed Aug 17, 2023
View reviewed changes
Comment thread.github/workflows/mypy.yml Outdated
AlexWaygood
AlexWaygood reviewed Aug 17, 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.

You might also want to alter .github/dependabot.yml, so that you get automated updates for the mypy version pin in Tools/cases_generator/requirements-dev.txt. Currently it only checks to see if dependencies in Tools/clinic/ need updating:

cpython/.github/dependabot.yml

Lines 15 to 21 in 75b3db8

- package-ecosystem: "pip"
directory: "/Tools/clinic/"
schedule:
interval: "monthly"
labels:
- "skip issue"
- "skip news"

gvanrossum
gvanrossum reviewed Aug 17, 2023
View reviewed changes
Comment threadTools/cases_generator/lexer.py Outdated
gvanrossum
gvanrossum reviewed Aug 17, 2023
View reviewed changes

@gvanrossumgvanrossum 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.

Thanks so much for doing this!

Comment threadTools/cases_generator/generate_cases.py Outdated
Comment thread.github/workflows/mypy.yml Outdated
Comment threadTools/cases_generator/analysis.py
Comment threadTools/cases_generator/generate_cases.py
Comment threadTools/cases_generator/lexer.py
gvanrossum
gvanrossum reviewed Aug 18, 2023
View reviewed changes

@gvanrossumgvanrossum 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.

(Please don't git push -f -- it makes the GitHub review reset so I can't do "changes since last review".)

Comment thread.github/workflows/lint.yml Outdated
Comment thread.github/workflows/mypy.yml Outdated
- uses: actions/checkout@v3
- uses: actions/setup-python@v4
with:
python-version: "3.x"

@gvanrossumgvanrossumAug 18, 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.

Are there constraints on x? The cases generator uses match/case so it's got to be at least Python 3.10.

@corona10corona10Aug 18, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Let's use 3.11. We need dog fooding

@hugovkhugovkSep 19, 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.

3.x is "the latest stable version of Python":

https://github.com/actions/setup-python/blob/main/docs/advanced-usage.md#specifying-a-python-version

That's 3.11 now, and will be 3.12 shortly after its release.

So 3.x would be fine to minimise churn on this line (for next time).

@corona10corona10Sep 19, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

So 3.x would be fine to minimise churn on this line (for next time).

Good!

Comment threadTools/cases_generator/generate_cases.py Outdated
Comment on lines +186 to +184
if popped is None and pushed is None:
popped, pushed = target_popped, target_pushed
else:
assert popped == target_popped
assert pushed == target_pushed
popped, pushed = target_popped, target_pushed

@gvanrossumgvanrossumAug 18, 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.

Those asserts need to stay -- per the comment on line 174-175, we're checking here that all targets of the pseudo have the same effects.

Comment threadTools/cases_generator/formatting.py Outdated
Comment threadTools/cases_generator/generate_cases.py
Comment threadTools/cases_generator/lexer.py
gvanrossum
gvanrossum reviewed Aug 18, 2023
View reviewed changes

@gvanrossumgvanrossum 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.

Thanks for taking black out.

gvanrossum
gvanrossum approved these changes Aug 18, 2023
View reviewed changes

@gvanrossumgvanrossum 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. Don't worry about the origin of the Iterator type.

@corona10

corona10 commented Aug 18, 2023•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Waiting comments of @AlexWaygood@hugovk before merging :)

erlend-aasland
erlend-aasland reviewed Aug 18, 2023
View reviewed changes
Comment threadTools/cases_generator/lexer.py

@erlend-aaslanderlend-aaslandAug 18, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

FYI: You could actually use Argument Clinic to generate the Python code for the keywords. (Argument Clinic supports Python files.) I'm not suggesting you do it for this PR, though; just a thought :)

@corona10corona10Aug 18, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Wow, Can I read the relevant documentation?

AlexWaygood
AlexWaygood reviewed Aug 18, 2023
View reviewed changes
Comment threadTools/cases_generator/mypy.ini Outdated
Comment threadTools/cases_generator/mypy.ini
corona10and others added 2 commits August 18, 2023 16:21
@corona10
Use shorter name of Iterator
5b3732d
@corona10@AlexWaygood
Apply suggestions from code review
aebfff3
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
AlexWaygood
AlexWaygood approved these changes Aug 18, 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! :D

@AlexWaygoodAlexWaygood changed the title gh-104504: Run mypy on cases_generatorgh-104504: Run mypy on cases_generator in CI (and blacken the code)Aug 18, 2023
@corona10
corona10 merged commit 28cab71 into python:mainAug 18, 2023
@corona10
corona10 deleted the gh-104504 branch August 18, 2023 13:42
@gvanrossum

gvanrossum commented Aug 18, 2023

Copy link
Copy Markdown
Member

Thanks!

@corona10corona10 mentioned this pull request Sep 19, 2023
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gvanrossumgvanrossumgvanrossum approved these changes
@erlend-aaslanderlend-aaslanderlend-aasland approved these changes
@AlexWaygoodAlexWaygoodAlexWaygood approved these changes
@ezio-melottiezio-melottiAwaiting requested review from ezio-melottiezio-melotti is a code owner
@iritkatrieliritkatrielAwaiting requested review from iritkatriel
@hugovkhugovkAwaiting requested review from hugovkhugovk is a code owner

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@corona10@gvanrossum@hugovk@erlend-aasland@AlexWaygood@bedevere-bot