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

Changed typing to work with the --disallow-any-generics flag on vistautils #73

Closed
wants to merge 2 commits into from

Conversation

jamart28
Copy link
Collaborator

@jamart28 jamart28 commented Feb 19, 2020

Closes #72

@jamart28
Copy link
Collaborator Author

I forgot to run make precommit and it seems that this raises an issue with immutable collections itself. I will work on fixing this.

Got rid of type alias IT and IT2
- Type aliases which use generics are themselves generics meaning they
receive implicit `Any`s unless the typing is specified. As such type
alias IT and IT2 are semi redundant as using them properly would be
`IT[KT, VT]` making the typing almost as long as using `Tuple` and
making it less readable (unless there was something there I didn't see)
@codecov
Copy link

codecov bot commented Feb 21, 2020

Codecov Report

Merging #73 into master will decrease coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #73      +/-   ##
==========================================
- Coverage   89.34%   89.31%   -0.04%     
==========================================
  Files           7        7              
  Lines         676      674       -2     
==========================================
- Hits          604      602       -2     
  Misses         72       72              

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 df73582...e7a3e63. Read the comment docs.

@jamart28
Copy link
Collaborator Author

@gabbard I'm still unable to request review from @lichmaster98.

@gabbard
Copy link

gabbard commented Feb 21, 2020

GitHub says he has not yet accepted the invite.

@lichtefeld lichtefeld self-requested a review February 21, 2020 15:44
@lichtefeld
Copy link
Collaborator

Apologies! I thought I had accepted this.

InstantiationTypes = (Mapping, Iterable) # pylint:disable=invalid-name


def immutabledict(
iterable: Optional[AllowableSourceType] = None, *, forbid_duplicate_keys: bool = False
iterable: Optional[AllowableSourceType[Any, Any]] = None,
Copy link

Choose a reason for hiding this comment

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

What happens if you use KT, VT instead of Any, Any?

Copy link
Collaborator Author

@jamart28 jamart28 Feb 21, 2020

Choose a reason for hiding this comment

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

It throws errors on vistautils saying that typing doesn't match. For some reason it tries to match to KT and VT exactly instead of using them as typevars. This can be seen as the original fix committed used typevars directly which mypy was okay with despite it being a syntax error on the typing. Furthermore, the errors thrown by mypy when using KT, VT were not errors from the flags so everyone who uses immutablecollections would be affected even if we just # type: ignoreed on vistautils. @gabbard

@gabbard
Copy link

gabbard commented Feb 25, 2020

Let's put this on hold until python/mypy#5738 is fixed.

@gabbard gabbard closed this Feb 25, 2020
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.

Implicit Any in immutabledict throws error on vistautils#86
3 participants