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

Bump mypy and fix compatibility issue with 0.920 #1503

Merged
merged 16 commits into from
Dec 21, 2021

Conversation

Apakottur
Copy link
Contributor

@Apakottur Apakottur commented Dec 17, 2021

Bump mypy to 0.920, and fix compatibility issues with the new version.

Details

Mypy introduced a breaking change in it's API in python/mypy#9951, where TypeVarDef and TypeVarType were merged into one class.

In addition to bumping the mypy version, we fix the compatibility issue with a simple check which preserves support for older versions of mypy.

This fix is similar to the one in Pydantic - pydantic/pydantic#3175

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Documentation

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

Sorry, something went wrong.

@botberry
Copy link
Member

botberry commented Dec 17, 2021

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:


This release adds support for mypy 0.920.


Here's the preview release card for twitter:

Here's the tweet text:

🆕 Release (next) is out! Thanks to Yossi Rozantsev for the PR 👏

Get it here 👉 https://github.com/strawberry-graphql/strawberry/releases/tag/(next)

@codecov
Copy link

codecov bot commented Dec 17, 2021

Codecov Report

Merging #1503 (adb9042) into main (fceeb71) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1503   +/-   ##
=======================================
  Coverage   98.11%   98.11%           
=======================================
  Files         128      128           
  Lines        4396     4396           
  Branches      746      746           
=======================================
  Hits         4313     4313           
  Misses         43       43           
  Partials       40       40           

@@ -8,7 +8,7 @@


async def await_maybe(value: AwaitableOrValue):
if inspect.iscoroutine(value):
if inspect.isawaitable(value):
Copy link
Member

Choose a reason for hiding this comment

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

Is this change related to the version bump?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, mypy 0.910 is ok with this but mypy 0.920 gives:
Incompatible types in "await" (actual type "CoroutineType", expected type "Awaitable[Any]").

We could alter the typing of the function to only accepts coroutines, instead of changing to isawaitable.

Copy link
Member

Choose a reason for hiding this comment

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

would you be come sending a separate PR just for this? 😊

I think it is worth isolating this change 😊 I also want to make sure there's no breaking change (even though our tests are passing)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, here's a separate PR - #1505 :)

@@ -507,6 +520,7 @@ def collect_attributes(self) -> Optional[List[DataclassAttribute]]:
line=stmt.line,
column=stmt.column,
type=sym.type,
kw_only=False,
Copy link
Member

Choose a reason for hiding this comment

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

Is this change related to the version bump?

Copy link
Member

Choose a reason for hiding this comment

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

Unrelated to this PR, but why do we use the dict constructor here instead of using a {} literal, @patrick91?

Copy link
Member

Choose a reason for hiding this comment

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

not 100% sure why this looks different than the mypy plugin: https://github.com/python/mypy/blob/master/mypy/plugins/dataclasses.py#L342

but if I did the change I used dict because I didn't want to change all the params to have quotes :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah the mypy dataclasses plugin added support for kw_only in python/mypy#10867, so we need it here as well.
I just put False here to see if things work. I'll compare our plugin to theirs and match the implementation.

strawberry/ext/mypy_plugin.py Outdated Show resolved Hide resolved
strawberry/utils/await_maybe.py Outdated Show resolved Hide resolved
RELEASE.md Outdated Show resolved Hide resolved
@@ -507,6 +520,7 @@ def collect_attributes(self) -> Optional[List[DataclassAttribute]]:
line=stmt.line,
column=stmt.column,
type=sym.type,
kw_only=False,
Copy link
Member

Choose a reason for hiding this comment

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

not 100% sure why this looks different than the mypy plugin: https://github.com/python/mypy/blob/master/mypy/plugins/dataclasses.py#L342

but if I did the change I used dict because I didn't want to change all the params to have quotes :D

@Apakottur Apakottur mentioned this pull request Dec 20, 2021
11 tasks
Copy link
Member

@patrick91 patrick91 left a comment

Choose a reason for hiding this comment

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

Looks great! thank you so much 😊

params["info"] = cls.info
attribute = DataclassAttribute(**params) # type: ignore
if MypyVersion.VERSION >= Decimal("0.920"):
params["kw_only"] = False
Copy link
Member

Choose a reason for hiding this comment

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

we'll need to change this after we merge: #1187

@Apakottur
Copy link
Contributor Author

I was able to run the plugin with mypy versions 0.920, 0.910 and 0.800. Should we try an older version of mypy in the tests, or is manual testing enough here?

@patrick91
Copy link
Member

I was able to run the plugin with mypy versions 0.920, 0.910 and 0.800. Should we try an older version of mypy in the tests, or is manual testing enough here?

should be fine I think, no need to make our ci more complicated 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants