Skip to content

Conversation

@bastimeyer
Copy link
Member

This finally adds the API documentation for plugin.api.validate. An API guide with real-life examples is not included yet and will be added later.

This was much more work than expected. Especially because of several sphinx-autodoc issues which required me to reformat and move everything multiple times.

I've tried to make the style as consistent as possible, but there are some inconsistencies between the validate docs, schema docs and utility function docs. These are just minor differences though and you won't notice them. I won't even tell you.

As explained in the top comment of validate.rst, the layout was chosen deliberately because of certain autodoc quirks. Unfortunately, it's not ideal, but it's the best I could come up with.


My first attempt was documenting all overloading function of validate() (base schema types, as well as custom schemas), but then I noticed that autodoc doesn't support singledispatch properly.

Manually importing the functions using autofunction also was a bad idea, because this would require duplicate docs, on both the schema validation functions and the schema classes themselves.

So I ended up with the current appoach of adding custom function docs in the validate.rst file for the basic schema validation functions like type, abc.Callable, list (etc), dict and re.Pattern, and importing the rest with proper docstrings in the python modules.

@bastimeyer

This comment was marked as outdated.

@bastimeyer
Copy link
Member Author

Copy link
Member

@gravyboat gravyboat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took a bit but I looked at everything. I don't see any typos or grammar issues. I've noted one small point about the doc formatting for builtins but it's not a show stopper nor does it need to be considered here. Looks really good!

Comment on lines +137 to +139
Please note the difference between :class:`list <builtins.list>`
and the :class:`ListSchema <_schemas.ListSchema>` validation.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential slight chance of confusion here as people may think you are referring to streamlink.plugin.api.validate.list which itself is an alias of ListSchema if they aren't familiar with the doc formatting. Not a big deal but it's easier to understand the difference for something like https://github.com/streamlink/streamlink/pull/5652/files#diff-010b6199b0fc1ebe5df5bdb5ee86c9d024fea4dca02b63f388fe11b1d5156599R216 because of the reference to re.pattern.

I wish there was a way <builtins.list> could easily link to the python docs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

people may think you are referring to streamlink.plugin.api.validate.list

Maybe, but if they check validate.list, they'll see that it's an alias for ListSchema, which means that this line compares the same things, which doesn't make much sense and it must be two different things. All members of the validate module are always referenced with their module name, never on their own, apart from schema class names.

I wish there was a way <builtins.list> could easily link to the python docs.

It's currently disabled. For a good reason, because it's way too noisy with external links everywhere...
https://github.com/streamlink/streamlink/blob/6.3.1/docs/conf.py#L108-L111

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me.

@bastimeyer
Copy link
Member Author

I'll fix the order of items later today or tomorrow and then merge. Any follow-up issues can be fixed afterwards.

@bastimeyer
Copy link
Member Author

Just to document this:

The reason why autodoc doesn't fully document the schema classes in the main module and instead outputs "alias of" is that the classes' __name__ attribute is different from the names of the streamlink.plugin.api.validate module members ("AllSchema" != "all").

This could be fixed by overriding the __name__ attribute of each class like this in __init__.py:

def _(items):
    for name, obj in items:
        if type(obj) is type and obj.__module__.startswith(f"{__name__}."):
            obj.__name__ = name


_(globals().items())
del _

But this would change the validation error message and it would require fixing the tests. I'll have a look at this in the future, because it also requires some changes in the docstrings in regards to the type annotations (the list issue mentioned above now being an actual issue).

@bastimeyer bastimeyer merged commit 7f4c86c into streamlink:master Nov 6, 2023
@bastimeyer bastimeyer deleted the docs/api/validate branch November 6, 2023 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants