Hide Nursery constructor, finalize and expose Nursery

[bengartner]

Issue: #1021

Seemed like a doable first issue for me since I've dabbled in metaclasses (and they classes were already written.)

Applied NoPublicConstructor to Nursery and added test cases for instantiation and subclassing. Worked pretty easily. Don't know the codebase well enough to know what else we might want to privatize. NurseryManager? CancelStatus? Do we want to protect everything or are there certain classes that are too magic to exist in nature?

[codecov]

Codecov Report

Merging #1090 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1090      +/-   ##
==========================================
+ Coverage   99.54%   99.54%   +<.01%     
==========================================
  Files         102      102              
  Lines       12501    12512      +11     
  Branches      950      950              
==========================================
+ Hits        12444    12455      +11     
  Misses         36       36              
  Partials       21       21

Impacted Files	Coverage Δ
trio/__init__.py	100% <ø> (ø)	⬆️
trio/_core/tests/test_run.py	100% <100%> (ø)	⬆️
trio/_core/_run.py	99.73% <100%> (ø)	⬆️

[njsmith]

We don't need to worry about special metaclasses for NurseryManager or CancelStatus, because those whole classes are private – you can only get them by doing trio._core._run.NurseryManager, and anyone who would do that already knows that they're cheating and isn't going to be stopped by having to write ._create as well :-). And they're going to stay private going forward.

So far, Nursery has also been kept private, but that's actually a problem (#778). The idea was to stop people instantiating it directly, and that part worked, but it also stops people from using it in type hints, or isinstance checks, and it makes it annoying to document because we can't just say "here are the docs for trio.Nursery", and so on.

This means that as it stands, this PR doesn't change anything in the public API – it takes something that was already hidden, and makes it even more hidden :-). But, it makes it possible to start exporting Nursery as part of the trio.* namespace. Do you want to do that too? Could be part of this PR, or could be a follow-up PR.

[njsmith]

Oh, and there are quite a few other classes where we might want to use these metaclasses – #1044 is a placeholder for figuring that out. I'll comment there with more details.

The actual change in this PR looks good. Going forward, I don't think we need to add this many tests every time we mark a class as Final or NoPublicConstructor – we already have tests for those metaclasses. But Nursery is such a core thing, and it's so important that people not mess around with it, that I don't mind having some special tests just for it.

[bengartner]

Do you want to do that too? Could be part of this PR, or could be a follow-up PR.

Yes. I haven't used the init and all patterns for making symbols public before but seems straightforward enough.

Unless we need a newsfragment or explicit test for the visibility of trio.Nursery.

[njsmith]

Yes. I haven't used the __init__ and __all__ patterns for making symbols public before but seems straightforward enough.

Yep, you got it.

Unless we need a newsfragment or explicit test for the visibility of trio.Nursery.

We don't need a test, but a newsfragment would be good. We should also add it to the docs.

The docs are more complicated than you might think, because we need to unwind the weird hack we're using to work around Nursery not being public. Usually, the way we arrange our docs is:

We put reference docs into docstrings on each class/function/method, using Google-style docstrings, and then in the sphinx manual we use the autodoc plugin to pull in the docstrings, and put more explanatory text around them.

For nursery, because it's not a public class, we couldn't do this, so instead we have some awkward workarounds. If you look at docs/source/reference-core.rst and search for .. interface:: The nursery interface, you'll see this weird thing:

• It's a .. interface::, which is a custom thing we invented just for this
• It has all the reference docs directly in the .rst file, instead of in the docstrings where they normally go
• They're all written in raw sphinx markup (using the :param: tag, etc.), instead of using Google docstring style (this is because autodoc knows how to process the google-style formatting, but sphinx proper doesn't, so if you don't use docstrings you can't use the google-style formatting)

So it would be good to move these docs into docstrings, convert to Google docstring style, and then pull them in with .. autoclass:: Nursery.

[bengartner]

Okay, I'll take a look at untangling the Nursery workaround.

[bengartner]

Okay, I understand what's going on with Nursery, but still figuring out the restructured text syntax. Notably Sphinx can't autoextract cancel_scope from the attribute list in the docstring because it's an instance variable and not a @Property. Feels like it should be document with the other attributes of Nursery though.

[bengartner]

It looks like those few lines are some sort of Sphinx boilerplate that you forgot to replace?

Yes, sorry, I have more edits but I accidentally pushed my draft version. Is that a "draft" or "do not merge" label I should apply while I'm working on it?

[njsmith]

Github has some kind of official way to mark a PR as "draft status" now, but I don't know if you can set it from an open PR – the docs just describe how to set it when creating a new PR.

The old convention is to edit the title to start with [wip], and then it's obvious to everyone that it's a work-in-progress

[bengartner]

I think this is ready now. (And I learned a lot about Sphinx.) Note that Nursery didn't have an actual docstring before, so someone should scrutinize that.

Not sure what's going on with the builds. Is Azure Pipelines having issues?

[njsmith]

The macOS failure is weird... might just be some kind of glitch?

The Travis CHECK_DOCS failure is:

/home/travis/build/python-trio/trio/trio/__init__.py:docstring of trio.open_nursery:1:Inline interpreted text or phrase reference start-string without end-string.

[pquentin]

Let's see if the macOS failure is a glitch or not by closing/reopening.

[bengartner]

Okay, I think I had some wires crossed after a rebase but I think this looks good now.

[pquentin]

Thank you! This looks really good:

Ironically we're now emphasizing the trio.Nursery constructor in the docs, but I don't think there's any way around that.

However, one thing that changed that we can probably improve is that start_soon is now at the bottom, but that's the method you want to show first. Ideally we could show start_soon, then start, then the three other things, as it's done currently. If there's no way to get cancel_scope after the two start methods, then so be it.

(@njsmith if you think none of that matters, please feel free to merge!)

[bengartner]

Ironically we're now emphasizing the trio.Nursery constructor in the docs, but I don't think there's any way around that.

Oh yeah, good point. And actually, I think I figured out how to deemphasize this. If I just pushed this change
.. autoclass:: Nursery => .. autoclass:: Nursery()
and that hides the constructor params. I think this feature is more for hiding optional parameters in the constructor but it seems to work for obscuring the constructor altogether.

[bengartner]

Ideally we could show start_soon, then start, then the three other things, as it's done currently.

Yeah, I just have to include the methods/attributes explicitly in the rst, I can do that.

[pquentin]

Nice find for the constructor!

Another option is to reorder the sources themselves, but I guess that introduces churn. Anyway, that's minor, and can always refine this later, so I'll merge this. Thank you!

[njsmith]

Nice catch on the nursery constructor :-)