Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve ngclient/metadata docs #1543

Closed
4 tasks
sechkova opened this issue Aug 26, 2021 · 7 comments
Closed
4 tasks

Improve ngclient/metadata docs #1543

sechkova opened this issue Aug 26, 2021 · 7 comments
Assignees
Labels
backlog Issues to address with priority for current development goals documentation Documentation of the project as well as procedural documentation
Milestone

Comments

@sechkova
Copy link
Contributor

Description of issue or feature request:

The documentation that will hopefully be hosted on readthedocs after the work on #1517 is done needs some further improvement:

  • Metadata API page is big, could be broken down for easier navigation
  • Class constructors have a lot of arguments which does not look good on the rendered page
  • A section for internal client details (TrustedMetadataSet, RequestsFetcher) could be useful
  • Expand the description of tuf.ngclient.fetcher (see Enable docs build with sphinx #1517 (comment))
@sechkova sechkova added the documentation Documentation of the project as well as procedural documentation label Aug 26, 2021
@sechkova
Copy link
Contributor Author

Some proposals from @jku:

Metadata API page is big, could be broken down for easier navigation

We could document the main classes (Metadata, all signed derivatives) page-per-class and then have one page for the "helper classes

Class constructors have a lot of arguments which does not look good on the rendered page

In general sphinx wants us to document our constructors in the class docstring and not in __init__ so we can work on that. Also this is one options for making the signature more readable.

autodoc_typehints = "description"
autodoc_typehints_description_target = "documented"

@jku
Copy link
Member

jku commented Aug 26, 2021

https://theupdateframework.readthedocs.io/en/stable/ is nothing yet: needs to be configured somehow I suppose?

EDIT: this also makes sense considering no release includes the config yet... they claim it's automatic:

We also create a stable version, if your project has any tagged releases. stable will be automatically kept up to date to point at your highest version.

so... let's get a release out

@sechkova
Copy link
Contributor Author

so... let's get a release out

Yes, exactly, I just tried manually building stable and the version it checked out is:
HEAD is now at 60875f91 Merge pull request #1284 from joshuagl/joshuagl/release-v0.17.0

@sechkova sechkova added the backlog Issues to address with priority for current development goals label Sep 1, 2021
@jku
Copy link
Member

jku commented Sep 11, 2021

Quick review of metadata doc:

  • As mentioned before, should make it clear that Metadata, Root, Targets, Snapshot and Timestamp are important, other stuff is helpers: An alternative to splitting to many pages may be to write a TOC in the beginning that makes those 5 stand out?
  • there's a lot of repetition of "tuf.api.metadata." in the signatures that is not required. add_module_names = False and python_use_unqualified_type_names = True fix most of the cases
  • to make the docs less complex we should try to hide classes that are implementation details: both Signed and Basefile should not be in public docs, we should try to use :inherited-members: in the relevant classes to make the API docs for the derived classes complete instead (basefile might not be need at all since it has no public API)
  • we probably don't need :show-inheritance: at all?
  • the attribute/method docs should get reviewed for "is this really useful for the developer"? Examples of less useful docstrings:
    • Dictionary of all unrecognized fields -- what is the purpose of this attribute?
    • Returns the dict representation of self -- why does the function exist?
  • filler words should be removed, arguably also obvious types should be removed from docstring as the signatures are fully type hinted:
    • An integer indicating the version of... -> Version of...
    • A string giving the name of ... -> Name of ...
  • _type should probably be hidden from docs (or, after the recent ADR on API design, removed from API entirely)
  • TargetFile.custom is not documented

And one more thing on ngclient:

  • We have TODOs for Error documentation: let's fill those in...

@jku
Copy link
Member

jku commented Sep 17, 2021

to make the docs less complex we should try to hide classes that are implementation details: both Signed and Basefile should not be in public docs, we should try to use :inherited-members: in the relevant classes to make the API docs for the derived classes complete instead (basefile might not be need at all since it has no public API)

This probably won't work: autodocs :inherited-members: does not apply to attributes so e.g. version attribute would not get documented in Root

@jku
Copy link
Member

jku commented Sep 17, 2021

sphinx autodoc can not automatically document the types of python instance attributes -- this makes sense when you think about the data autodoc has available (for functions the types are annotated but for attributes it would have to run a static type check to figure the types out)

Since in Metadata API we have documented the attributes, and not constructor arguments (because they are typically the same), we are now in this documentation situation:

  • constructor arguments with type hints but no documentation
  • attributes with documentation but no types

(╯°□°)╯︵ ┻━┻

Even if we document the the attribute type in a way sphinx understands, it uses huge amounts of vertical space per attribute: it just looks quite awful with the sort of classes we have. I think I might suggest documenting the arguments (which are dcoumented nicely with types and in reasonable vertical space), and just stating that they are also instance attributes...

@jku
Copy link
Member

jku commented Oct 1, 2021

#1590 is merged, looks a lot better I think.

Followup happens in #1312, #1595, #1599, #1600, #1601, #1602. Closing this one.

@jku jku closed this as completed Oct 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog Issues to address with priority for current development goals documentation Documentation of the project as well as procedural documentation
Projects
None yet
Development

No branches or pull requests

2 participants