merge GithubObject.pyi/Requester.pyi stubs back to source #2463
Conversation
trim21
changed the title
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## master #2463 +/- ##
==========================================
- Coverage 98.50% 98.43% -0.08%
==========================================
Files 128 128
Lines 12757 12806 +49
==========================================
+ Hits 12566 12605 +39
- Misses 191 201 +10
☔ View full report in Codecov by Sentry. |
trim21
changed the title
Co-authored-by: Enrico Minack <github@enrico.minack.dev>
|
can you consider #2522 , so the ci will failed with better message? now pre-commit or mypy will all failed in py38 job |
Co-authored-by: Enrico Minack <github@enrico.minack.dev>
| @@ -136,45 +127,43 @@ def edit( | |||
| :param note_url: string | |||
| :rtype: None | |||
| """ | |||
| assert scopes is github.GithubObject.NotSet or all( | |||
| assert isinstance(scopes, _NotSetType) or all( | |||
There was a problem hiding this comment.
I really don't like how this degenerates from
assert scopes is NotSet
to
assert isinstance(scopes, _NotSetType)
@JLLeitschuh any suggestions?
There was a problem hiding this comment.
why it's a degenerate...
There was a problem hiding this comment.
It's going from a constant comparison using is to an isinstance check, which means that the entire type hierarchy needs to be checked every time now, right?
There was a problem hiding this comment.
Oh yes, it impact performance.
But I don't think it's very necessary
and also UnSet is Attribute[None] now.
There was a problem hiding this comment.
I mean this aesthetically.
assert scopes is NotSet
Is readable,
assert isinstance(scopes, _NotSetType)
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.
But python typing system doesn't support this.
If I use emun as single value here, it can't pass mypy:
o: Attribute[Optional[int]] = NotSetThere was a problem hiding this comment.
Also Protocol doesn't support generic, so make Attribute a Protocol won't work.
I don't know if there is any way to avoid all these problem.
There was a problem hiding this comment.
There is a good side of this, now you can write assert isinstance(v, (_NotSetType, str)) instead of assert v is NotSet or instance(v, str)
There was a problem hiding this comment.
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
Co-authored-by: Jonathan Leitschuh <jonathan.leitschuh@gmail.com>
This reverts commit 4447be1.
ref #2462