Add tests, add docstrings, and fix small bugs #28
Conversation
consideRatio
force-pushed
the
pr/refresh-and-add-tests
branch
from
d6cff3c to
1acc764
Compare
consideRatio
added
bug
consideRatio
force-pushed
the
pr/refresh-and-add-tests
branch
from
1acc764 to
5747e4b
Compare
consideRatio
changed the title
There was a problem hiding this comment.
This is truly wonderful, thanks for putting in all the effort!
Looking through the examples, I think we want to be able to support these cases:
- document all config options on the class, but no Python and no inheritance (I think this is what
autoconfigurablewithout members does now) - document all config options including inherited config, still without Python APIs (I think this is infeasible because
:inherited-members:also pulls in Python members, so you'd have to identify and manually exclude all non-configurable members, right?
Maybe the answer is to not use :inherit-members:, and instead define a new option like :inherit-config-members:. I'm not sure about needing a :config-members: equivalent to :members:, because autoconfigurable doesn't make any sense if you aren't going to document any configurable traits, since that's just the same output as autoclass, right?
I don't think you need to address these things here, because what you have is already a massive improvement. I think having these wonderful examples and illustrations merely reveals that these are cases we should think about. So maybe it should be an Issue. Then again, nobody's asked for the inherit-config-but-not-python case, so maybe it doesn't matter.
|
|
||
| .. autoconfigurable:: test_module.TestConfigurableSubclass | ||
| :noindex: | ||
| :inherited-members: |
There was a problem hiding this comment.
I tried to set :inherited-members: traitlets.HasTraits, since the documentation suggests this should exclude methods inherited from this class, but it doesn't seem to work.
I've tried it on autoclass without autodoc-traits, and :inherited-members: parent.Class seems to simply not do what it says.
There was a problem hiding this comment.
I recall reading upstream sphinx source code and noting its used as a boolean flag rather than a list of members.
There was a problem hiding this comment.
HMmm, what version of sphinx was used? See sphinx-doc/sphinx#10325 added for sphinx 5.0
There was a problem hiding this comment.
5.3.0. Not our problem, for now. It seems like it will be desirable pretty often for documenting inheritors of HasTraits, which has a lot of methods.
|
Actually, now that I think about it, the inherited-members being backwards means we've always been showing inherited config and never showing just on-class. This is the case in jupyterhub, because log_level is inherited from base Application and is documented. I think that's probably what folks usually want, so maybe it is important to keep it working, and maybe even still be the default. What do you think is the best way to signal that? |
|
Sorry for mixed messages - that doesn't mean I think it needs to block this PR, but I think it does mean we need to open an Issue about it that blocks release so we can make an explicit decision about it. |
Co-authored-by: Min RK <[email protected]>
for more information, see https://pre-commit.ci
The current behavior before and after the PR adding this commit, is that also the inherited configurable traitlets will be presented. This commit make us test against a class with inherited configurable traitlets to clarify that.
|
@minrk I've addressed all comments, either by adding commits or commenting. On my end, this is ready for a merge but are pinging you to re-review added commits. |
|
This is terrific, great work! |
|
Thanks for reviewing this @minrk!!! |
I've worked to understand, document, test, and fix smaller bugs I found.
About tests
The tests/docs folder contains a sphinx project with examples used in the tests, which are overall simple should provide quite good coverage. They ensure that the sphinx directives are executed in various situations without warnings, and render .html strings of relevance.
It could have made sense to not render all the way to .html, but instead for example inspect the rST emitted by the directives somehow - but as I didn't knew how to do that I went for this end-to-end like test which probably is a good starting point to have for anyone looking to rework this further.
About small bugs
:inherited-members:options was passed to anautoconfigurabledirective, but I don't think anyone use that as it clutters things.Closes The
inherited-membersSphinx directive option is respected backwards #19:exclude-members:was bypassed, but will now be respected.autoclasscould be used instead ofautoconfigurable- as it can't.Closes
autoclassis not the same asautoconfigurable- but should be? #26About documentation
Understanding autodoc_traits requires understanding of details of how sphinx.ext.autodoc works internally, which lacks documentation. Due to that I've invested a lot of time to help my future self and other contributors understand autodoc_traits by providing relevant references to sphinx source code and comments to summarize the important parts.
Notes for review
I've probably invested ~16-24 hours into this. It took quite a while to onboard myself as I had to learn from comments in the sphinx source code to understand how things worked in more detail.
I ended up adding multiple labels to this PR as it was quite extensive, which will make it show up in multiple categories if github-activity is used. A lot of the refactoring was very relevant documenting, but some refactoring also led to bugfixes etc.