[Tidy] Remove docstrings from build methods to harmonize overall

[maxschulz-COL]

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)

Notice

• I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":

  • I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.
  • I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorized to submit this contribution on behalf of the original creator(s) or their licensees.
  • I certify that the use of this contribution as authorized by the Apache 2.0 license does not violate the intellectual property rights of anyone else.
  • I have not referenced individuals, products or companies in any commits, directly or indirectly.
  • I have not added data or restricted code in any commits, directly or indirectly.

[antonymilne]

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.

[maxschulz-COL]

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]

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]

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

[stichbury]

LGTM 🥇