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

Set python_version and fix uppercase builtins #1577

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

christianbundy
Copy link
Contributor

@christianbundy christianbundy commented Jun 24, 2023

Problem

When upgrading to Mypy 1.4 (#1572) I added a setting to force uppercase builtins to ensure that tests would pass. In a comment I highlighted that we may not be able to remove this setting without changing python_version otherwise tests would only pass on some versions of Python.

I tried setting python_version = 3.8, but that caused test failures. Candidly, I didn't take very much time to investigate these.

Solution

Configure python_version = 3.11 to match our highest supported Python version, and fix uppercase builtins. I can fix the union types in a future PR, but I want to keep this one as minimal as possible. If this is the direction we want to go, we can merge this PR, but I'm open to other options.

@christianbundy christianbundy changed the title Set python_version = 3.8 Set python_version Jun 24, 2023
@christianbundy christianbundy changed the title Set python_version Set python_version and fix uppercase builtins Jun 24, 2023
@christianbundy christianbundy marked this pull request as ready for review June 24, 2023 17:41
@christianbundy
Copy link
Contributor Author

Hmm. This is failing locally too -- I'm not sure why.

=================================== FAILURES ===================================
_______________________ models_triple_circular_reference _______________________
/home/runner/work/django-stubs/django-stubs/tests/typecheck/fields/test_related.yml:252: 
E   pytest_mypy_plugins.utils.TypecheckAssertionError: Invalid output: 
E   Actual:
E     ...
E     main:3: error: "User" has no attribute "profile" (diff)
E     main:3: note: Revealed type is "Any"          (diff)
E   Expected:
E     ...
E     main:3: note: Revealed type is "myapp.models.profile.Profile" (diff)
E   Alignment of first line difference:
E     E: main:3: note: Revealed type is "myapp.models.profile.Profile"
E     A: main:3: error: "User" has no attribute "profile"
E                ^

@@ -15,8 +15,8 @@ disallow_untyped_defs = true
disallow_incomplete_defs = true
show_error_codes = false
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note that show_error_codes = false also exists here as legacy, and should be fixed eventually.

You can move it below the # TODO comment you have added.

Copy link
Collaborator

@intgr intgr left a comment

Choose a reason for hiding this comment

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

Thanks for the effort and sorry that this PR has languished without review for so long. Clearly there is room for improvement in how we handle PRs.

@@ -15,8 +15,8 @@ disallow_untyped_defs = true
disallow_incomplete_defs = true
show_error_codes = false
disable_error_code = empty-body
python_version = 3.11
Copy link
Collaborator

@intgr intgr Oct 17, 2023

Choose a reason for hiding this comment

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

python_version also determines the value of sys.version_info used when interpreting typeshed stubs (for conditionals such as https://github.com/python/typeshed/blob/main/stdlib/typing.pyi#L93-L113)

So if we override this setting, we lose test coverage with different variations of typeshed definitions, and would no longer catch subtle compatibility issues in CI.

I very much prefer the modern formatting of types in error messages, but still I would err on the side of caution and prioritize test coverage here.

I wish mypy had a way to override force_uppercase_builtins in the opposite direction, e.g. force_lowercase_builtins = true. Maybe they would be willing to entertain such a PR if someone is interested doing the work. And same for force_union_syntax.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I agree - we cannot touch python_version :(

@intgr intgr added the discussion needed Issues or PRs that require further discussion or a decision to be made label Oct 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion needed Issues or PRs that require further discussion or a decision to be made
Development

Successfully merging this pull request may close these issues.

3 participants