Fix #21

[SimonBoothroyd]

This PR aims to fix #21 in a backwards compatible way.

It

• updates the index builder to handle either a dict or list of children
• updates the api doc signature patch to handle a list of nodes rather than a list of strings.

@jbms I wasn't sure why the api doc patch is removing properties from the parts list, and this line doesn't seem to be hit when building the example docs in the repo. Any ideas if we need it?

For now I just return the output of the original function and that seems to yield docs that look identical.

[2bndy5]

I wasn't sure why the api doc patch is removing properties from the parts list, and this line doesn't seem to be hit when building the example docs in the repo. Any ideas if we need it?

I'm guessing this was a style choice on behalf of @jbms. The line is likely not hit in the docs build because there isn't any decorated instance attributes in demo python API which was copied from sphinx-rtd-theme docs.

I added an instance attribute to the doc's demo python API to trigger this line.

class Foo:
    def __init__(self, qux, spam=False):
        # ...

        #: Doc comment for instance attribute qux.
        self.qux = 3

    # not currently present in demo API
    @property
    def is_foobar(self) -> bool:
        """Docstring for instance attribute."""
        return True

The results are:

using sphinx v4.2

using sphinx v4.3

I'd vote to not remove "properties" from the signature (in sphinx v4.2) because there is a very significant distinction between decorated vs un-decorated instance attributes.

[SimonBoothroyd]

Ah that makes sense!

I'd vote to not remove "properties" from the signature (in sphinx v4.2) because there is a very significant distinction between decorated vs un-decorated instance attributes.

+1

[jbms]

Yes, the reason for removing "property" from the signature line was a stylistic choice.

It wasn't so much to improve the regular full object description, but rather to improve the custom autosummary used in tensorstore:
https://google.github.io/tensorstore/python/api/tensorstore.IndexTransform.html#accessors

I thought it was nicer stylistically, and also easier to read, when all of the names (be they methods or properties) lined up, similar to doxygen class listings.

Additionally, I would agree that the distinction between static and instance properties/attributes is important (so static is still retained), but the distinction between a property and regular data attributes did not seem important, as that is really just an implementation detail.

I suppose one problem is that if you have something like:

class Foo:
    x = 10

assert Foo.x == 10
foo = Foo()
assert foo.x == 10
foo.x = 100
assert foo.x == 100
assert Foo.x == 10

Then you can access x both as a "static" property of Foo but it can also serve as a default value for an x data attribute of Foo instances.

[jbms]

In fact currently static is retained which interferes with the horizontal alignment of members in the autosummary:

https://google.github.io/tensorstore/python/api/tensorstore.Promise.html

I think it might be better to indicate static methods (and other things like that) on a separate line rather than as a prefix, using the more Python-like syntax

@staticmethod
foo(a: T, b: U) -> V

The @staticmethod part would be shown in a different color than the name.

I suppose another option would be to modify the prefixes only for the autosummary, not for full object descriptions, though I think there is an advantage to keeping the styling uniform as well.

[2bndy5]

This is what the python docs say about the builtin property()

---

Then you can access x both as a "static" property of Foo but it can also serve as a default value for an x data attribute of Foo instances.

Ultimately, it is up to the doc's author to provide insight on the behavior of an object's member. Conventionally speaking, read-only properties should indicate (via docstr) that they are read-only. Otherwise, the user could run into a NotImplementedError

>>> obj = Foo()
>>> obj.is_foobar
True
>>> obj.is_foobar = False
NotImplementedError: is_foobar does not have a setter

The python docs do steer against using class attributes, but like you said, it is an implementation detail...

I do like the idea of putting the signature prefix on its own line. This could be something to add to the resolution of #14 (for all language syntax).

[jbms]

Thanks. I have merged this for now to fix the documentation build, although I am still inclined to remove the property prefix since, as mentioned, it seems to me that it is just an implementation detail not really relevant to users of the API.

Strangely it appears that github actions are still not being run for pull requests.

[2bndy5]

it seems to me that it is just an implementation detail not really relevant to users of the API

If its a style choice, then the prefix removal should be abstracted to your customized implementation of autodoc extension. To remove it goes against the norm of documenting python APIs...

I already offered my opinion about how it is relevant to API users. But, just for clarity, the "property" prefix indicates that it isn't just another variable you can set/get like you would any other python variable. Rather, it should indicate that the attribute's implementation has implications that should be considered before using it like any other python variable.

[2bndy5]

pylint complains:

sphinx_immaterial/search_adapt.py:32:8: C0206: Consider iterating with .items() (consider-using-dict-items)

---

Strangely it appears that github actions are still not being run for pull requests.

I still think there's something up with this repo's settings. Also, note that approval to run workflows for first time contributors is not the same as approval of changes in the PR.

[jbms]

I fixed the remaining issues, including the lint error, in #24.

Per your request to retain the property prefix, I will see about adding a config option to control that rather than doing it unconditionally.

I intend to add the custom autosummary and autodoc changes from tensorstore, and also the json domain, into this theme as well (as optional features).

[jbms]

As far as github actions I'm still not sure what the problem is. For this pull request I don't think there even was a prompt to approve running actions.