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

Import type information directly from the typing module. #657

Closed
wants to merge 2 commits into from
Closed

Import type information directly from the typing module. #657

wants to merge 2 commits into from

Conversation

TonyRippy
Copy link

Hello, thank you for all your hard work on drf-spectacular!

At Materialize, we currently use pyright as our static type checker. In the most recent update, we ran into an issue (microsoft/pyright#3044) that the pyright authors are declining to fix. As a result OpenApiParameter.QUERY, PATH, etc. are not being typed correctly and fail validation.

This PR works around the issue by importing from the typing module directly instead of indirectly through the drainage module. Is this something you'd be willing to consider?

This is done to work around microsoft/pyright#3044, which doesn't recognize types imported indirectly through another module.
@codecov
Copy link

codecov bot commented Feb 15, 2022

Codecov Report

Merging #657 (7fd0a2a) into master (27ac1ca) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #657   +/-   ##
=======================================
  Coverage   98.80%   98.80%           
=======================================
  Files          63       63           
  Lines        7005     7009    +4     
=======================================
+ Hits         6921     6925    +4     
  Misses         84       84           
Impacted Files Coverage Δ
drf_spectacular/drainage.py 98.83% <ø> (-0.04%) ⬇️
drf_spectacular/plumbing.py 97.58% <100.00%> (+0.01%) ⬆️
drf_spectacular/utils.py 99.50% <100.00%> (+<0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 27ac1ca...7fd0a2a. Read the comment docs.

@tfranzel
Copy link
Owner

Hi @TonyRippy

thank you! So I actually doubled down on that an hour ago 😆, which is why there is a conflict now.

You surely noticed that we do this so we don't have those conditional imports all over the place. Single source of truth and all that jazz. As you mentioned in that other issue, my first take would be that pyright should get this right. Pretty sure we are not alone in doing this.

  • Is this only about Final and Literal and what other typing members are affected by this?
  • Is pyright only complaining for util or also for plumbing?

@TonyRippy
Copy link
Author

TonyRippy commented Feb 15, 2022

Thanks for taking a look!

  • Is this only about Final and Literal and what other typing members are affected by this?

AFAIK, it's only the Final -> Literal mapping that requires special handling. I'm not aware of problems with other typing members.

  • Is pyright only complaining for util or also for plumbing?

I only noticed pyright failures in util. In particular it would complain that the OpenApiParameter constants did not meet the _ParameterLocationType type constraint.

@hmc-cs-mdrissi
Copy link

microsoft/pyright#3025 (comment) is the full list of typing members that are affected by this issue.

Literal, TypeAlias, Annotated, Final, ClassVar, Required, NotRequired

@tfranzel
Copy link
Owner

Hey @TonyRippy, please review #658. i took the liberty of doing a minimal change while hopefully accommodating pyright. Let me know if that works for you.

tfranzel added a commit that referenced this pull request Feb 16, 2022
accommodate pyright limitations #657
@tfranzel
Copy link
Owner

superceded by #658

@tfranzel tfranzel closed this Feb 16, 2022
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