FazBrowse GitHub Viewer | Trending | | Home

Tools: [Download Repo ZIP] [Original HTTPS Page]

gh-104504: Run mypy on cases_generator in CI (and blacken the code) by corona10 · Pull Request #108090 · python/cpython · GitHub

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

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

Copy link Copy Markdown

Member

Issue: Run mypy on cases_generator #104504

bedevere-bot mentioned this pull request Aug 17, 2023

Run mypy on cases_generator #104504

Closed

corona10 added the skip news label Aug 17, 2023

corona10 marked this pull request as ready for review August 17, 2023 16:55

corona10 requested review from ezio-melotti and hugovk as code owners August 17, 2023 16:55

bedevere-bot added the awaiting core review label Aug 17, 2023

corona10 requested review from AlexWaygood, gvanrossum and iritkatriel August 17, 2023 16:55

corona10 commented Aug 17, 2023 •
edited
Loading

Copy link Copy Markdown

Member Author

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

Add type hints where needed
Add static symbols(OVERRIDE, REGISTER, IF) where needed
Remove unreachable codes
Found a bug!: https://github.com/python/cpython/pull/108090/files#r1297496975

corona10 commented Aug 17, 2023

View reviewed changes

Comment thread Tools/cases_generator/generate_cases.py

corona10 commented Aug 17, 2023

View reviewed changes

Comment thread Tools/cases_generator/analysis.py

corona10 commented Aug 17, 2023

Copy link Copy Markdown

Member Author

AlexWaygood reviewed Aug 17, 2023

View reviewed changes

Comment thread Tools/cases_generator/requirements-dev.txt Outdated

AlexWaygood reviewed Aug 17, 2023

View reviewed changes

Comment thread Tools/cases_generator/mypy.ini Outdated

AlexWaygood reviewed Aug 17, 2023

View reviewed changes

Comment thread Tools/cases_generator/requirements-dev.txt Outdated

AlexWaygood reviewed Aug 17, 2023

View reviewed changes

Comment thread Tools/cases_generator/formatting.py Outdated

Comment thread Tools/cases_generator/generate_cases.py Outdated

hugovk reviewed Aug 17, 2023

View reviewed changes

Comment thread .github/workflows/mypy.yml Outdated

AlexWaygood reviewed Aug 17, 2023

View reviewed changes

AlexWaygood left a comment

Copy link Copy Markdown

Member

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 reviewed Aug 17, 2023

View reviewed changes

Comment thread Tools/cases_generator/lexer.py Outdated

gvanrossum reviewed Aug 17, 2023

View reviewed changes

gvanrossum left a comment

Copy link Copy Markdown

Member

Thanks so much for doing this!

Comment thread Tools/cases_generator/generate_cases.py Outdated

Comment thread .github/workflows/mypy.yml Outdated

Comment thread Tools/cases_generator/analysis.py

Comment thread Tools/cases_generator/generate_cases.py

Comment thread Tools/cases_generator/lexer.py

corona10 force-pushed the gh-104504 branch from 594be7a to 56a9fdc Compare August 18, 2023 00:12

corona10 requested a review from erlend-aasland as a code owner August 18, 2023 00:12

corona10 force-pushed the gh-104504 branch from 1b41b98 to 689768e Compare August 18, 2023 00:14

corona10 added 4 commits August 18, 2023 09:19

pythongh-104504 : Run mypy on cases_generator

6f4ab6e

Add mypy.ini

c0fb645

Address code review

840dab9

Update

6c89a68

corona10 force-pushed the gh-104504 branch from 3325794 to 6c89a68 Compare August 18, 2023 00:20

Move lint

42ed502

corona10 requested review from gvanrossum and hugovk August 18, 2023 00:30

corona10 added 2 commits August 18, 2023 09:37

Update

999ba10

revert

5078f7a

gvanrossum reviewed Aug 18, 2023

View reviewed changes

gvanrossum left a comment

Copy link Copy Markdown

Member

(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"

gvanrossum Aug 18, 2023

Copy link Copy Markdown

Member

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

corona10 Aug 18, 2023

Copy link Copy Markdown

Member Author

Let's use 3.11. We need dog fooding

hugovk Sep 19, 2023

Copy link Copy Markdown

Member

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).

corona10 Sep 19, 2023

Copy link Copy Markdown

Member Author

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

Good!

Comment thread Tools/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

gvanrossum Aug 18, 2023

Copy link Copy Markdown

Member

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 thread Tools/cases_generator/formatting.py Outdated

Comment thread Tools/cases_generator/generate_cases.py

Comment thread Tools/cases_generator/lexer.py

gvanrossum reviewed Aug 18, 2023

View reviewed changes

gvanrossum left a comment

Copy link Copy Markdown

Member

Thanks for taking black out.

corona10 added 6 commits August 18, 2023 10:27

Address code review

e695d2f

Resolve redundant-expr

a136c24

Use matrix

d26b39e

update

c0ae7c2

Address code review

dcb3ada

Fix indent

7f9e68b

corona10 requested a review from gvanrossum August 18, 2023 02:44

gvanrossum approved these changes Aug 18, 2023

View reviewed changes

gvanrossum left a comment

Copy link Copy Markdown

Member

LGTM. Don't worry about the origin of the Iterator type.

bedevere-bot added awaiting merge and removed awaiting core review labels Aug 18, 2023

corona10 commented Aug 18, 2023 •
edited
Loading

Copy link Copy Markdown

Member Author

Waiting comments of @AlexWaygood @hugovk before merging :)

run black

9b13d6b

erlend-aasland reviewed Aug 18, 2023

View reviewed changes

Comment thread Tools/cases_generator/lexer.py

erlend-aasland Aug 18, 2023

Copy link Copy Markdown

Contributor

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 :)

corona10 Aug 18, 2023

Copy link Copy Markdown

Member Author

Wow, Can I read the relevant documentation?

erlend-aasland approved these changes Aug 18, 2023

View reviewed changes

AlexWaygood reviewed Aug 18, 2023

View reviewed changes

Comment thread Tools/cases_generator/mypy.ini Outdated

Comment thread Tools/cases_generator/mypy.ini

corona10 and others added 2 commits August 18, 2023 16:21

Use shorter name of Iterator

5b3732d

Apply suggestions from code review

aebfff3

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

AlexWaygood approved these changes Aug 18, 2023

View reviewed changes

AlexWaygood left a comment

Copy link Copy Markdown

Member

LGTM, thanks! :D

AlexWaygood changed the title gh-104504: Run mypy on cases_generator gh-104504: Run mypy on cases_generator in CI (and blacken the code) Aug 18, 2023

corona10 merged commit 28cab71 into python:main Aug 18, 2023

bedevere-bot removed the awaiting merge label Aug 18, 2023

corona10 deleted the gh-104504 branch August 18, 2023 13:42

gvanrossum commented Aug 18, 2023

Copy link Copy Markdown

Member

Thanks!

corona10 mentioned this pull request Sep 19, 2023

[3.12] gh-109125: Run mypy on Tools/wasm (GH-109126) #109561

Closed

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters

Show hidden characters

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

Reviewers

gvanrossum approved these changes

erlend-aasland approved these changes

AlexWaygood approved these changes

Awaiting requested review from ezio-melotti ezio-melotti is a code owner

Awaiting requested review from iritkatriel

Awaiting requested review from hugovk hugovk is a code owner

Assignees

No one assigned

Labels

Projects

Milestone

No milestone

Back | FazBrowse Home | New Git URL