| msg293592 - (view) |
Author: Nathaniel Smith (njs) *  |
Date: 2017-05-13 02:48 |
| sphinxcontrib-trio [1] does a few things; one of them is to enhance sphinx's autodoc support by trying to sniff out the types of functions so that it can automatically determine that something is, say, a generator, or an async classmethod. This runs into problems when it comes to context managers, for two reasons. One reason is pretty obvious: the sniffing logic would like to be able to tell by looking at a function that it should be used as a context manager, so that it can document it as such, but there's no reliable way to do this. The other reason is more subtle: the sniffing code has to walk the .__wrapped__ chain in order to detect things like async classmethods, but when it walks the chain for a @contextmanager function, it finds the underlying generator, and ends up thinking that this function should be documented as returning an iterator, which is just wrong. If we can detect context managers, we can know to ignore any Obviously this is always going to be a heuristic process; there's no 100% reliable way to tell what arbitrary wrappers are doing. But the heuristic works pretty well... it's just that right now the method of detecting context manager functions is pretty disgusting: https://github.com/python-trio/sphinxcontrib-trio/blob/2d9e65187dc7a08863b68a78bdee4fb051f0b99e/sphinxcontrib_trio/__init__.py#L80-L90 https://github.com/python-trio/sphinxcontrib-trio/blob/2d9e65187dc7a08863b68a78bdee4fb051f0b99e/sphinxcontrib_trio/__init__.py#L241-L246 So it would be really nice if contextlib.contextmanager somehow marked its functions because: - then I could (eventually) use that marker instead of making assumptions about contextmanager's internals and messing with code objects - then we could (immediately) point to how the standard library does it as a standard for other projects to follow, instead of using this weird sphinxcontrib-trio-specific __returns_contextmanager__ thing that I just made up. I don't really care what the marker is, though I suppose __returns_contextmanager__ = True is as good as anything and has the advantage of a massive installed base (2 projects). [1] http://sphinxcontrib-trio.readthedocs.io/ |
|
|
| msg293596 - (view) |
Author: Alyssa Coghlan (ncoghlan) * |
Date: 2017-05-13 05:51 |
| Given that we have contextlib.AbstractContextManager now, perhaps it should just set "f.__annotations__['return'] = AbstractContextManager"? (assuming no other return annotation is already set) |
|
|
| msg293964 - (view) |
Author: Ivan Levkivskyi (levkivskyi) * |
Date: 2017-05-19 18:53 |
| Or you can use typing.ContextManager[ret_type] if you like generics (typing.AsyncContextManager will be also added soon). Also this recent discussion seems relevant https://github.com/python/peps/pull/242 and the corresponding thread on python-dev: https://www.mail-archive.com/python-dev@python.org/msg95595.html |
|
|
| msg295328 - (view) |
Author: Nathaniel Smith (njs) * |
Date: 2017-06-07 10:21 |
| I can think of two downsides to using __annotations__ for this: 1) Obviously contextlib itself isn't going to add any kind of annotation in any versions before 3.7, but third-party projects might (like contextlib2, say). And these projects have been known to be used on Python versions that don't have __annotations__ support. At the moment I personally don't have to care about this because sphinxcontrib-trio doesn't support anything before 3.5, but I suspect this will eventually change. (E.g. sphinx has expressed some interest in having these changes upstreamed.) So this would mean that we'd still need some other ad hoc mechanism to use when running on old Pythons, and have to check for both that and the __annotations__. But that's probably doable, so eh. It'd be helpful if contextlib2 could join in on whatever protocol, though. 2) sphinxcontrib-trio actually interacts very poorly with __annotations__ right now [1]. But I mean, I need to figure out how to fix this anyway, so... not really your problem :-). Neither of these downsides seems very compelling :-). So I guess contextlib should add some appropriate __annotations__, and *maybe* also add something like __returns_contextmanager__ = True if that's useful to maintain consistency with contextlib2 or similar? (Wasn't there some discussion about keeping type hints out of the stdlib for now? is this likely to ruffle any figures on those grounds?) [1] https://github.com/python-trio/sphinxcontrib-trio/issues/4 |
|
|
| msg295333 - (view) |
Author: Alyssa Coghlan (ncoghlan) * |
Date: 2017-06-07 11:59 |
| Even Python 2 functions support setting arbitrary attributes, so contextlib2.contextmanager can always just add an __annotations__ dict to decorated functions if it doesn't already exist. ``` >>> def f(): ... pass ... >>> f.__annotations__ Traceback (most recent call last): File "", line 1, in AttributeError: 'function' object has no attribute '__annotations__' >>> f.__annotations__ = {} >>> f.__annotations__ {} ``` |
|
|
| msg295334 - (view) |
Author: Alyssa Coghlan (ncoghlan) * |
Date: 2017-06-07 12:03 |
| As far as whether or not this usage fits within the guidelines at https://www.python.org/dev/peps/pep-0008/#function-annotations, it would definitely be worth bringing up in a python-dev thread before going ahead with it. I think it's a good dynamic use of annotations that fits within the spirit of PEP 484 without requiring sphinxcontrib-trio to rely on a static typechecker like mypy, but others may have a different perspective. |
|
|