Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
merge GithubObject.pyi/Requester.pyi stubs back to source #2463
merge GithubObject.pyi/Requester.pyi stubs back to source #2463
Changes from all commits
1c27b76
e857eb2
47e4268
54c447b
a441a80
a1b4956
20368a1
daf26cf
138ef0f
dc88329
c3f8d6f
2a27864
e04cfee
12812b5
21c4f10
944d963
dbcdaa4
d6087f7
1e6e5c7
78af79a
21656b3
de351b5
e1bde06
599f483
a21eb6b
6b79429
a6d27c6
c1e81f7
7b28c24
1accb10
703b864
2d19cf3
cf7854d
25a3081
e235af6
37a98bc
1cff8f9
6d4f636
3696407
8906d8f
fa29df6
cab0709
5e925f8
823f9f6
cd94401
ab380f0
4447be1
276701c
1ffb057
1cf2f46
ee1340c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really don't like how this degenerates from
to
@JLLeitschuh any suggestions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why it's a degenerate...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's going from a constant comparison using
is
to anisinstance
check, which means that the entire type hierarchy needs to be checked every time now, right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
python/typing#236
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes, it impact performance.
But I don't think it's very necessary
and also
UnSet
isAttribute[None]
now.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean this aesthetically.
Is readable,
is ugly.
Plus, we need to import a private class
_NotSetType
. A lot of code has to be touched. The former was present before.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But python typing system doesn't support this.
If I use emun as single value here, it can't pass mypy:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also
Protocol
doesn't support generic, so makeAttribute
aProtocol
won't work.I don't know if there is any way to avoid all these problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a good side of this, now you can write
assert isinstance(v, (_NotSetType, str))
instead ofassert v is NotSet or instance(v, str)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @trim21 here. There may not be a way to make the type checker happy moving forward. Im inclined to let us poke at trying to make this better in a followup PR
This file was deleted.