Skip to content

Do not require typing_extensions on python 3.8+ - #12

Merged
squahtx merged 1 commit into
matrix-org:mainfrom
paper42:typing-extensions-req
Dec 16, 2021
Merged

squahtx merged 1 commit into
matrix-org:mainfrom
paper42:typing-extensions-req

Conversation

@paper42

@paper42 paper42 commented Nov 28, 2021

Copy link
Copy Markdown
Contributor

Protocol is a part of typing since python 3.8 https://docs.python.org/3/library/typing.html#typing.Protocol

@paper42
paper42 requested a review from a team as a code owner November 28, 2021 13:15
@squahtx

squahtx commented Nov 29, 2021

Copy link
Copy Markdown
Contributor

Reducing dependencies is a good idea, but mypy doesn't seem to like the construct:

signedjson/types.py:21: error: Incompatible import of "Protocol" (imported name has type "typing_extensions._SpecialForm", local name has type "typing._SpecialForm")

https://mypy-play.net/?mypy=latest&python=3.10&gist=9ac99f23480e5c8bc86c68f94ce65b6c

@squahtx
squahtx requested review from squahtx and removed request for a team November 29, 2021 10:36
@DMRobertson

Copy link
Copy Markdown
Contributor

Reducing dependencies is a good idea, but mypy doesn't seem to like the construct:

python/mypy#1153 (comment) describes a neater workaround

@paper42

paper42 commented Dec 15, 2021

Copy link
Copy Markdown
Contributor Author

I am really sorry about the delay, mypy, pylint and flake8 now pass on my changes.

@squahtx squahtx left a comment

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.

Thank you for making the changes!

Looks good, just two minor comments.

Comment thread setup.py
'typing_extensions>=3.5;python_version<"3.8"',
'typing>=3.5;python_version<"3.5"',
'importlib_metadata;python_version<"3.8"',
],

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.

I think we always need typing_extensions as a dev dependency now.

Suggested change
],
],
extras_require={
"dev": ["typing_extensions>=3.5"],
},

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Is that because the linters would find an unmet dependency?

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.

Yes, when running mypy, we'd always use typing_extensions now

Comment thread signedjson/types.py
@squahtx squahtx self-assigned this Dec 16, 2021
@paper42
paper42 force-pushed the typing-extensions-req branch from 3a9bae6 to 3258a36 Compare December 16, 2021 10:51

@squahtx squahtx left a comment

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.

Looks good to me! Thanks for the PR!

@squahtx
squahtx merged commit 0ad43d0 into matrix-org:main Dec 16, 2021
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.

3 participants