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 schema version docstrings #137

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

omus
Copy link
Member

@omus omus commented Dec 7, 2022

While updating some other Julia packages to use Legolas 0.5 I noticed a pattern where documentation for Legolas schema versions ended up just copying the schema version definition into the docstring. That style of docstring isn't very human friendly so I decided to try out some of the concepts we've been playing with in a private package here for consideration.

Changes made:

  • Placed docstrings before @version definitions. This still attaches the docstring to the struct and matches the typical Julia docstring being placed above the definition (Legolas 0.5 doesn't allow you to attach the docstring directly to the @version macro ATM)
  • Improve the description of the Onda defined schema versions to mention the Julia type itself is just implementing of a specification
  • Break out the required fields as separate bullet points which make it much easier to read. This provides us a way to add descriptions to each field
  • When schema versions inherit fields I'm referencing the documentation of that schema version to make it easy to determine the complete set of fields needed to be defined by the Legolas schema.

@ericphanson
Copy link
Member

ericphanson commented Dec 8, 2022

We could maybe make these docstrings autogenerated if we use inline docstrings in the @version invocation, like

@version AnnotationV1 begin
    "The UUID identifying the recording with which the annotation is
  associated."
    recording::UUID = UUID(recording)
    "The UUID identifying the annotation."
    id::UUID = UUID(id)
    "The annotation's time span within the recording."
    span::TimeSpan = TimeSpan(span)
end

Also, is "required fields" the right heading? Since all fields are required now I think

@omus
Copy link
Member Author

omus commented Dec 8, 2022

We could maybe make these docstrings autogenerated if we use inline docstrings in the @version invocation

I like this idea. I think there is still a use case however for adding a description in a docstring. Maybe we can generate a default docstring and if a description is needed one could do:

"""
    AnnotationV1

My amazing description.

$(required_fields_docs(AnnotationV1))
"""
@version AnnotationV1 begin
    "The UUID identifying the recording with which the annotation is associated."
    recording::UUID = UUID(recording)

    "The UUID identifying the annotation."
    id::UUID = UUID(id)

    "The annotation's time span within the recording."
    span::TimeSpan = TimeSpan(span)
end
I validated that this approach is actually feasible
julia> docs(x) = "# Heading\n\nFunction named `$(repr(x))`"
docs (generic function with 1 method)

julia> f
ERROR: UndefVarError: f not defined

julia> """
          f

       My amazing docstring.

       $(docs(f))
       """
       f(x) = x
f

help?> f
search: f fd for fma fld fld1 fill fdio frexp foldr foldl flush floor float first fill! fetch fldmod filter falses finally foreach fldmod1 findmin findmax findall

  f

  My amazing docstring.

  Heading
  ≡≡≡≡≡≡≡≡≡

  Function named f

Also, is "required fields" the right heading? Since all fields are required now I think

Possibly it isn't the right heading as any field accepting missing would be considered optional during construction.

Update: DocStringExtensions.jl pretty much does exactly what we want. I can see some issues with how Legolas handles parameterization and possibly wanting to see the expression used to convert the value to the field.

@jrevels
Copy link
Member

jrevels commented Dec 16, 2022

this could be updated against beacon-biosignals/Legolas.jl#74 now :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants