mckinsey / vizro

Vizro is a toolkit for creating modular data visualization applications.
https://vizro.readthedocs.io/en/stable/
Apache License 2.0
2.46k stars 109 forks source link

[Tidy] Remove docstrings from build methods to harmonize overall #495

Closed maxschulz-COL closed 1 month ago

maxschulz-COL commented 1 month ago

Description

Our API docs looked confusing because two model (Action and Layout) had docstrings for the build method. I converted them to comments, so no info is lost, but now our docs look more streamlined again.

Screenshot

(left before - missed Action, right after)

Screenshot 2024-05-23 at 09 38 05 Screenshot 2024-05-23 at 09 43 10

Notice

maxschulz-COL commented 1 month ago

Agree we should change this because it currently looks weird, but I think there's probably a better fix than this: leave the docstrings how they are and just edit the settings in models.md to exclude the build method:

::: vizro.models.types
    options:
      filters: ["!^_"]  # Don't show dunder methods as well as single underscore ones
      merge_init_into_class: false

Presumably just adding build to the list of filters or similar would work here.

Tbh then I think all build functions should have a docstring, because they are on equal footing.

I think this change is the more consistent and easier one... And I don't think build functions that are never publically used should necessarily need to be documented

antonymilne commented 1 month ago

Tbh then I think all build functions should have a docstring, because they are on equal footing.

I don't think so - some build functions are more complicated, and hence worthy of documentation, than others. Most build functions are pretty much self-explanatory, but if it's useful to have some extra comments then we shouldn't be prevented from doing so - that documentation is just for our purposes rather than general users. And I think it makes more sense to put that documentation in a docstring because that is the place a developer expects to find it, rather than in an inline comment.

I think this change is the more consistent and easier one... And I don't think build functions that are never publically used should necessarily need to be documented

Agree they don't necessarily need documenting in general, which is why most of them don't have a docstring. But I don't think this should stop us from adding a docstring in the cases where it is useful.

maxschulz-COL commented 1 month ago

I don't think so - some build functions are more complicated, and hence worthy of documentation, than others. Most build functions are pretty much self-explanatory, but if it's useful to have some extra comments then we shouldn't be prevented from doing so - that documentation is just for our purposes rather than general users. And I think it makes more sense to put that documentation in a docstring because that is the place a developer expects to find it, rather than in an inline comment.

Agree, but I think our current docstring choice was at best random. Some of the most complicated build methods have comments, while the two above mentioned are actually fairly simply. So I was more aiming to have a constistant documentation level.

Agree they don't necessarily need documenting in general, which is why most of them don't have a docstring. But I don't think this should stop us from adding a docstring in the cases where it is useful.

Agreed, and that's maybe the main reason why a filter is good. Still think the current docstring choice is inconsistent, but can change back if you think those two in particular we should keep