Jump to conversation
Unresolved conversations (7)
@glyph glyph Mar 15, 2021
Hmm. This `cast` seems like it's a problem, but probably not *this* PR's problem.
src/twisted/internet/defer.py
glyph wsanchez
@glyph glyph Mar 15, 2021
Also docstring here.
src/twisted/internet/defer.py
@glyph glyph Mar 15, 2021
This needs a docstring, which will hopefully also make it stop complaining about coverage.
src/twisted/internet/defer.py
@glyph glyph Mar 15, 2021
Can you file / link a ticket for this?
src/twisted/internet/defer.py
@glyph glyph Mar 15, 2021
Is there a mypy bug we could link to here?
src/twisted/internet/defer.py
glyph graingert
@glyph glyph Mar 15, 2021
I don't understand the issue being ignored here. Isn't this compatible with `Awaitable.__await__` ? That would seem to be the whole point?
Outdated
src/twisted/internet/defer.py
wsanchez glyph
@wsanchez wsanchez Feb 12, 2021
Repeating a comment from @mstojcevich in @glyph's suggestion PR here so we don't lose it: > Is None correct for the generator/coroutine return type for these? I think it should be Optional[object] since this handles pulling the value off of StopIteration(x) when the generator returns x. > > And I don't think YieldType is necessarily a Deferred when gen is a Generator. > > I think the same concerns apply to the original PR fwiw.
src/twisted/internet/defer.py
Resolved conversations (28)
@glyph glyph Mar 15, 2021
"... but we should gradually enable them as time allows" :)
mypy.ini
wsanchez
@glyph glyph Mar 15, 2021
Nitpick: new test names should be `test_wordsWordsWords`. Not gonna block on that though ;-)
Outdated
src/twisted/test/test_defer.py
glyph wsanchez
@glyph glyph Mar 15, 2021
We can add this to `.coveragerc` to avoid this type of bogus coverage gap: ``` [report] exclude_lines = pragma: no cover if TYPE_CHECKING \s*\.\.\.$ ```
src/twisted/internet/interfaces.py
glyph wsanchez
@glyph glyph Mar 15, 2021
Seems like this should also be a generic, and this should be `Deferred[T]`? If users are using it for something heterogenous, `T` can always be `Any`.
Outdated
src/twisted/internet/defer.py
@glyph glyph Mar 15, 2021
Do we support a version of Python where this `AttributeError` can still happen? Can we just nuke the whole `try`?
Outdated
src/twisted/internet/defer.py
wsanchez
@glyph glyph Mar 15, 2021
Ugh I can't wait to drop 3.6 so we don't have all this gratuitous quoting
src/twisted/internet/defer.py
wsanchez
@glyph glyph Mar 15, 2021
What? Why does mypy think these types are distinct at all? It should be able to see them as a pure type alias?
src/twisted/internet/defer.py
wsanchez
@glyph glyph Mar 15, 2021
If we want to support this behavior, we should be testing it. Rather than a `ignore[unreachable]`, specify something like `onTimeoutCancelLegacy: Optional[Callable[[object, float], object] = onTimeoutCancel` up at the top of the function, then explain that we don't want it publicly typed to allow None but we may want to allow non-mypy users to keep passing it
Outdated
src/twisted/internet/defer.py
wsanchez
@wsanchez wsanchez Feb 11, 2021
See above; this was moved up in the file
src/twisted/internet/defer.py
@glyph glyph Jan 29, 2021
This code path is extremely hot, so I'd be a little wary of adding the two extra `getattr` calls here. (I wish speed center could quickly just, you know, tell us the performance impact instead of guessing like this, but c'est la vie)
Outdated
src/twisted/internet/defer.py
wsanchez glyph
@glyph glyph Jan 29, 2021
If you can get rid of this modification you can avoid worrying about the codecov error here ;-)
Outdated
src/twisted/internet/base.py
wsanchez
@glyph glyph Jan 29, 2021
I think that the right answer here is to have `defer.fail` explicitly become a `Deferred[Any]`, so that it will match shapes like this one.
Outdated
src/twisted/internet/base.py
wsanchez
@wsanchez wsanchez Nov 2, 2020
This to avoid a function that sets an attribute on itself
Outdated
src/twisted/internet/defer.py
wsanchez glyph
@wsanchez wsanchez Oct 26, 2020
Old thing has false positive for "merge failure" check
Outdated
README.rst
@glyph glyph Oct 16, 2020
In addition to self-as-typevar having some issues with mypy's internals, there's also the issue here that this signature is conceptually wrong too; `_DeferredT.addCallback(x->y)` does *not* return `_DeferredT` in the future generic version, it returns `Deferred[Y]`. I think if we want to go with simpler stubs for the first iteration, it would be better to simply type this as `Deferred`. (I think the idea here is to capture `DeferredList`'s subclass behavior, but `DeferredList` being a subclass ought to be irrelevant; the public attributes `fireOnOneCallback` etc. are exposed by accident from the constructor, ought to be private, and it has no differing public behaviors beyond those.
Outdated
src/twisted/internet/defer.py
wsanchez glyph
@glyph glyph Oct 16, 2020
Why make this (incorrectly) unreachable rather than making the arguments properly `Optional`?
Outdated
src/twisted/internet/defer.py
wsanchez glyph
@wsanchez wsanchez Oct 15, 2020
OK perhaps this looks like a weird change. The tests [`OtherPrimitivesTests.testLock`](https://github.com/twisted/twisted/blob/10017-mypy-defer/src/twisted/test/test_defer.py#L2131) and [`OtherPrimitivesTests.testSemaphore`](https://github.com/twisted/twisted/blob/10017-mypy-defer/src/twisted/test/test_defer.py#L2191) pass in `self=` to this method, which presumably means that `kwargs` here needs to accept any argument, hence the original method signature, which lacked `self` entirely, and the funky argument handling removed below to get it back. That, of course, makes it hard for a tool like `mypy` to know what's going on with the method signature. So here we put both `self` and the required first argument (which we can now type as a `Callable`) back in, but then we have to deal with the need to be able to pass `self` in as a keyword to the callable… So we change the name of `self`, and we want that name not to be some other thing you might want to pass into the callable. So I took a UUID and encoded it to letters. If someone ends up trying to pass `dbjAAcAiBbiFeBiEAcjgDhfFchjEBahF` as a keyword here then I HATE THAT JERK SHUT UP GO AWAY. So there.
Outdated
src/twisted/internet/defer.py
glyph wsanchez
graingert
@graingert graingert Oct 14, 2020
```suggestion def __iter__(self) -> _DeferredT: ``` `Deferred` already has a `__next__`
Outdated
src/twisted/internet/defer.py
wsanchez
@graingert graingert Oct 14, 2020
an enum might be better here ``` class _Sentinel(enum.Enum): NO_RESULT = auto() CONTINUE = auto() ``` then you can use `Union[DeferredCallback, Literal[_Sentinel.CONTINUE]]`
Outdated
src/twisted/internet/defer.py
mthuurne wsanchez
@wsanchez wsanchez Oct 14, 2020
This suggests that `Generator[Deferred, object, None]]` is the wrong return type for `f`. I could use other eyes on this.
src/twisted/internet/defer.py
@wsanchez wsanchez Oct 14, 2020
I feel like I must be doing something wrong here but I don't see it
Outdated
src/twisted/internet/defer.py
wsanchez graingert
@wsanchez wsanchez Oct 14, 2020
This to avoid a function that sets an attribute on itself
Outdated
src/twisted/internet/defer.py
@wsanchez wsanchez Oct 14, 2020
old code sure likes to pretend that `False is 0` and `True is 1`
src/twisted/internet/defer.py
@wsanchez wsanchez Oct 14, 2020
This one is only used internally.
Outdated
src/twisted/internet/defer.py
@wsanchez wsanchez Oct 14, 2020
These are standard for `*args` and `**kwargs`… so less compelling for clients, and maybe overkill here, but I think they help with reading the intended uses.
src/twisted/internet/defer.py
@wsanchez wsanchez Oct 14, 2020
These two are used in public type signatures and I thought they would be useful to clients, so I didn't prefix them with `_`… but I'm not super happy with these signatures, since they aren't providing a good descriptions due to the limitations of `Callable`. I think they remain useful in that when we eventually fix these, the user of these variables will then benefit from the checking. I used the "next best" examples above when doing the initial typing work do distinguish callbacks from errbacks, for example, but we can't ship with those definitions because they don't allows for callbacks with extra arguments.
Outdated
src/twisted/internet/defer.py
wsanchez glyph
graingert
@wsanchez wsanchez Oct 14, 2020
Moved up for easier to manage definition order
src/twisted/internet/defer.py
@wsanchez wsanchez Oct 14, 2020
Should all imports be prefixed with '_' or is '_MethodType' special? It would be safer to move to `_foo.py` files with `foo.py` importing the public symbols if we are trying to keep the namespace cleaner than "look at `__all__`", no?
src/twisted/python/compat.py
wsanchez glyph
graingert