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

Add api and style section about documentation #8

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

tingerrr
Copy link
Member

@tingerrr tingerrr commented Jun 3, 2024

This PR adds both a style and an API section on documentation comments, the style section takes inspiration from Tidy's syntax and suggestions by the tinymist author regarding simplicity.

I'd like to get some input from @Mc-Zen and @Myriad-Dreamin regarding syntax.

I have not yet included things like tests, examples and other annotations, and I'd like to know what you guys think are sensible in the constraints of LSP and Tidy.

It would be great if we could choose a formal syntax for documentation that both LSPs and doc different parses can understand.

@Myriad-Dreamin
Copy link

Is there rendered document? I have to render the document in my brain.

@tingerrr
Copy link
Member Author

Is there rendered document? I have to render the document in my brain.

I'll add one to the branch for previewing.

@tingerrr
Copy link
Member Author

See sections II.1.9 and III.1.3.

Copy link

@Myriad-Dreamin Myriad-Dreamin left a comment

Choose a reason for hiding this comment

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

I have made some comments. There are many typos, but I think it is still at draft stage so I ignore them.

src/chapters/style/documentation.typ Outdated Show resolved Hide resolved
src/chapters/api/documentation.typ Show resolved Hide resolved
src/chapters/api/documentation.typ Outdated Show resolved Hide resolved
src/chapters/api/documentation.typ Show resolved Hide resolved
src/chapters/style/documentation.typ Outdated Show resolved Hide resolved
src/chapters/style/documentation.typ Outdated Show resolved Hide resolved
src/chapters/style/documentation.typ Outdated Show resolved Hide resolved
src/chapters/style/documentation.typ Outdated Show resolved Hide resolved
src/chapters/style/documentation.typ Outdated Show resolved Hide resolved
@Myriad-Dreamin
Copy link

I'm wondering whether a module can be documented like rust.

//! This is a
//! module-level documentation.

@tingerrr
Copy link
Member Author

I'm wondering whether a module can be documented like rust.

//! This is a
//! module-level documentation.

This is something I considered, as it is at least required for the entry point of a package.

@Mc-Zen
Copy link

Mc-Zen commented Jun 12, 2024

Hi, looks pretty good to me (except for the typos :D). I agree with most things that @Myriad-Dreamin said. Section II.1.9 could do with a typical example for a docstring already. The syntax specification for the semantic trailer is very precise but hard-to-read, especially for non-programmers.

Note that tidy currently doesn't require a leading space after the three forward slashes - except for the parameter documentation (which allows at most one leading space before the parameter name, but on that subject, see below). But I admit that it looks hella ugly without the space, so this should definitely be encouraged.

Parameter types should probably really be annotated like (str | float | length) instead of (str, float, length). However, it is as it is currently and this part is not important anyway as parameter type annotations will move into the language. The same holds for return type annotations.

A suggestion concerning parameter documentation

I like this trailer block approach as you call it because it keeps the documentation together which has some advantages.

It recently occured to me, however, that - from a Typst syntax point of view - the most natural syntax for parameter description lists would really be term lists since this is semantically very "correct".

/// [description]
///
/// / x: This is the first argument
/// / y: This is the second argument and
///       of course we can break the line. 

This is also in line with adding special options via a #property(...) function. The difference is really just replacing - with / but suddenly it is valid Typst syntax (what a coincidence).

I somehow like the idea that doc comments are (almost) valid Typst.

@Mc-Zen
Copy link

Mc-Zen commented Jun 12, 2024

I'm wondering whether a module can be documented like rust.

//! This is a
//! module-level documentation.

In the current (unpushed) version of tidy, I'm testing support for module descriptions. They look just the same, i.e.,

/// This module is a very interesting and useful module. Note that it
/// is documented with a simple doc comment. 

The difference is just that it is not directly followed by a #let or let binding (and it should also probably come before any code, but a license comment should definitely be allowed before it).

I like simplicity where possible.

@Mc-Zen
Copy link

Mc-Zen commented Jun 12, 2024

Oh btw: For the entire guidelines: could the Don't boxes be colored orange for example, in contrast to the turquoise ("typst"-colored) Do boxes?

PS: I love the style of the document, looks really beautiful.

@tingerrr
Copy link
Member Author

I agree with most of what you said in your first comment. I'm unsure about this:

This is also in line with adding special options via a #property(...) function. The difference is really just replacing - with / but suddenly it is valid Typst syntax (what a coincidence).

Can you elaborate on this? I'm unsure how these relate exactly.

In the current (unpushed) version of tidy, I'm testing support for module descriptions. They look just the same, i.e.,

I'm a little on edge about this, those comments looking the exact same can easily cause confusion, don't you think? On the other hand, maybe a few blank lines are simple enough to tell this apart.

Oh btw: For the entire guidelines: could the Don't boxes be colored orange for example, in contrast to the turquoise ("typst"-colored) Do boxes?

Yeah, I've not really done any styling yet other than choosing a template, I'll adjust these at a later date and focus on the content first.

PS: I love the style of the document, looks really beautiful.

Thanks, most of the styling is actually by @jneug, as I'm currently (ab)using mantys for this document. I'll switch to the fork typst-community/mantodea mainly because it doesn't need a lot of the stuff mantys offers and requires a different structure. I'll upstream some of my fixes to mantys once it's done, but like I said, the appearance is not of concern to me yet.

@tingerrr
Copy link
Member Author

Regarding the EBNF of the doc comments, I will probably move this into an appendix and use simpler examples to make it easier to read. But it should be noted that this is aimed at developers of packages and templates, so my assumption is that a reader is not a complete novice when it comes to programming.

A simpler document for beginners which goes over typst ecosystem basics could be written at some point.

@Mc-Zen
Copy link

Mc-Zen commented Jun 13, 2024

Can you elaborate on this? I'm unsure how these relate exactly.

I meant that using term lists would be a step further towards doc comments being practically valid Typst syntax instead of some "made-up" syntax. Similar to using #property(..) for making advanced annotating instead of inventing a new syntax (that might then collide with existing Typst syntax).

I'm a little on edge about this, those comments looking the exact same can easily cause confusion, don't you think? On the other hand, maybe a few blank lines are simple enough to tell this apart.

Indeed, the idea is to have some blank lines. Basically it would be just a doc comment standing alone as opposed to being attached to a definition. Especially if the file starts with such a comment, I think there is no ambiguity attached. This is similar to like it's handled by Doxygen (strictly, Doxygen requires you to specify the @file macro but that's out-of-style here imo).

@tingerrr
Copy link
Member Author

@Mc-Zen is there any technical reason tidy doesn't simply use @val instead of @@var? As far as I can tell, you could define a show rule which intercepts all references in doc rendering directly and resolves them if possible or simply returns them as is. This would avoid enforcing special syntax for referring to other items.

You could even allow markdown like trailing syntax to reroute short labels to more complex expressions:

#import "internal.typ"

/// See @func               // ref to self
/// See also @internal:func // ref to internal.func
/// See also @other[func]   // also ref to internal.func displayed as "func"
///
/// / <other>: internal:func
#let func(..args) = {
  internal.func(..args)
}

This is similar to how rust allows:

use internal;

/// See [func]                // ref to self
/// See also [internal::func] // ref to internal::func
/// See also [func][other]    // also ref to internal::func displayed as "func"
///
/// [other]: internal::func
fn func(args: ()) = {
  internal::func(args)
}

The only reason I can think of to not do this would be that it's complicated to implement the resolving, as tidy doesn't know where the relative links lead to. But tidy could enforce that only absolute links from the root are valid and require the full paths of items to be defined so they create the full labels.

@tingerrr
Copy link
Member Author

I've gone over most of the comments, taking the liberty to enforce more modern syntax which can later be implemented by LSPs and doc parsers.

Properties

Properties need some refining, should they maybe be key value pairs instead?

/// #property(access: "private")

instead of

/// #property("private")

This makes the interpretation clearer, I'm fine with leaving what exactly is conventions vs what is custom na package specific up to doc parsers and LSPs, but some of these should be standard for sure like:

  • deprecation
    • possibly including a version since and/or until
    • a reason and/or suggestion
    • an alternative function for an LSP replace action (a lá "replace deprecated call with new version?" or similar)
  • visibility
    • public
    • private
    • public but unstable
    • etc
  • contexual
    • a boolean
    • or a string description for more complex contextuality requirements like context fallbacks for certain parameters

Optionality

I've also taken the liberty to define a parameter's description as non optional, if it's public it should be documented.

Cross References

As seen in my previous comment I'm a little concerned about custom sytnax for corss references when there is technically no need for it. See my above comment for this.

Semantic Trailer

The parsing for the semantic trailer cna be quite complex, I still think we could make this easier if we required parameter docs to be on the parameters themselves. I think this has the following benefits:

  • docs blocks become smaller and easier to maintian in isolation
  • avoidance of custom syntax for argument lists simplifies trailer sytnax and increases flexibility

Let me know what you guys think.

@tingerrr tingerrr force-pushed the tingerrr/zkvkylymtwyl branch from 538c4b0 to 67fa480 Compare June 16, 2024 14:46
@Mc-Zen
Copy link

Mc-Zen commented Jun 18, 2024

Properties need some refining, should they maybe be key value pairs instead?

I would prefer something like this too. Standard values can also be defined at a later point when it becomes clearer what is necessary and used often.

I've also taken the liberty to define a parameter's description as non optional, if it's public it should be documented.

If you say! (It should be self-evident anyways but might help to make it clear).

As seen in my previous comment I'm a little concerned about custom sytnax for corss references when there is technically no need for it. See my above comment for this.

I'll comment on this separately.

The parsing for the semantic trailer cna be quite complex, I still think we could make this easier if we required parameter docs to be on the parameters themselves. I think this has the following benefits:

Mmh, I tried it out and it's actually not bad. You also don't need to repeat the parameter name. However, it might be a bit early still because it leaves no way for type annotations (currently). I will look into an implementation for supporting it.

One downside with this option is that it makes putting the documentation into separate files (with just the signatures, not the definitions) harder. Some people might prefer this to reduce the code size and thus overall package size (for very large packages with extensive documentation).

@Mc-Zen
Copy link

Mc-Zen commented Jun 18, 2024

@Mc-Zen is there any technical reason tidy doesn't simply use @Val instead of @@var? As far as I can tell, you could define a show rule which intercepts all references in doc rendering directly and resolves them if possible or simply returns them as is. This would avoid enforcing special syntax for referring to other items.

So, the reason is that tidy prepends some prefix to all labels to distinguish them from "normal" labels and references. References are also resolved by applying the prefix. The idea is to avoid collisions for once but also to allow using normal references as well. Just plain @ syntax would of course be nice. I will check out what is possible, also between for cross-referencing between modules.

@Mc-Zen
Copy link

Mc-Zen commented Jun 18, 2024

@tingerrr

Okay, so far one issue:
The short-hand @ syntax only supports a very limited number of characters in the label name. For example, all kinds of parentheses cannot be used, so @func() or @other[func] does not work. This is a bit unlucky. Tidy enforces adding parentheses to function cross-references (e.g., @@func()) since version 0.2.0. This has the advantage that the displayed reference can distinguish between variables and functions which I find very helpful for the reader. Maybe there is another way to store this information? Is it possible to find out whether a label exists?

@tingerrr
Copy link
Member Author

One downside with this option is that it makes putting the documentation into separate files (with just the signatures, not the definitions) harder. Some people might prefer this to reduce the code size and thus overall package size (for very large packages with extensive documentation).

I'm unsure what you mean by that, you can put the docs elsewhere in tidy?.

Okay, so far one issue:
The short-hand @ syntax only supports a very limited number of characters in the label name. For example, all kinds of parentheses cannot be used, so @func() or @other[func] does not work. This is a bit unlucky. Tidy enforces adding parentheses to function cross-references (e.g., @@func()) since version 0.2.0. This has the advantage that the displayed reference can distinguish between variables and functions which I find very helpful for the reader. Maybe there is another way to store this information? Is it possible to find out whether a label exists?

Regarding this, yes you can, simply query for it in a ref show rule before showing something.

I still don't understand why reading the source would require knowing whether a function or value was referenced, this distinction is kind of superficial. In the end, we can easily distinguish it in the actual rendered output easily.

I have worked on some building blocks for items which have a definition site and many reference sites (like a function definition in the docs and references to it) yo can view that here. It doesn't have an up-to-date ref image, but it essentially renders the following:
image

The keys are arbitrary, but the parentheses are not required to know this is a function, as this information is stored in a state and handled by the registered "handlers". Assuming the @@ syntax is kept to disambiguate from normal labels, this would still work.

@Mc-Zen
Copy link

Mc-Zen commented Jun 20, 2024

I'm unsure what you mean by that, you can put the docs elsewhere in tidy?.

Technically, yes. I had thought about this a long time ago already. You can practically write files like this

/// here comes the documentation
/// - arg1 (int): ...
#let func(arg1, arg2: 1)

that don't not contain the implementation but only the signature and declaration and feed those into the docs generator.

Regarding this, yes you can, simply query for it in a ref show rule before showing something.

Okay, this would be done via state then?

I still don't understand why reading the source would require knowing whether a function or value was referenced, this distinction is kind of superficial. In the end, we can easily distinguish it in the actual rendered output easily.

No not in the source I meant, just in the output!

@tingerrr
Copy link
Member Author

Technically, yes. I had thought about this a long time ago already. You can practically write files like this

Ok, I'm sure there is a use case for this, but I generally would avoid it because it requires extra care to sync up the docs with changes to the API all the time. I would not make this part of the guidelines, but leave it as a tidy specific feature.

Okay, this would be done via state then?

Yes

No not in the source I meant, just in the output!

Well, as you can see above, the distinction in the source itself is not required at all.

@tingerrr
Copy link
Member Author

tingerrr commented Jul 9, 2024

I have fixed some spelling and grammar mistakes, but I would appreciate a review for both that and the general syntax choices and such.

@tingerrr tingerrr force-pushed the tingerrr/zkvkylymtwyl branch from 767ac2e to 57b8407 Compare July 9, 2024 12:03
@Myriad-Dreamin
Copy link

mark

Copy link

@Mc-Zen Mc-Zen left a comment

Choose a reason for hiding this comment

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

Nice, it is very thorough and covers everything I can think of.

I've added some comments and marked a few typos

src/chapters/style/documentation.typ Outdated Show resolved Hide resolved
src/chapters/style/documentation.typ Outdated Show resolved Hide resolved
src/chapters/style/documentation.typ Outdated Show resolved Hide resolved
src/chapters/style/documentation.typ Outdated Show resolved Hide resolved
src/chapters/style/documentation.typ Outdated Show resolved Hide resolved
src/chapters/api/documentation.typ Outdated Show resolved Hide resolved
src/chapters/api/documentation.typ Outdated Show resolved Hide resolved
src/chapters/api/documentation.typ Outdated Show resolved Hide resolved
src/chapters/api/documentation.typ Outdated Show resolved Hide resolved
src/chapters/api/documentation.typ Outdated Show resolved Hide resolved
Copy link

@Myriad-Dreamin Myriad-Dreamin left a comment

Choose a reason for hiding this comment

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

I put some comments.

src/chapters/style/documentation.typ Outdated Show resolved Hide resolved
src/chapters/style/documentation.typ Outdated Show resolved Hide resolved
src/chapters/style/documentation.typ Outdated Show resolved Hide resolved
src/chapters/style/documentation.typ Outdated Show resolved Hide resolved
src/chapters/style/documentation.typ Outdated Show resolved Hide resolved
src/chapters/style/documentation.typ Outdated Show resolved Hide resolved
src/chapters/style/documentation.typ Outdated Show resolved Hide resolved
src/chapters/style/documentation.typ Outdated Show resolved Hide resolved
/// / arg (types): Description for arg
/// -> types
///
/// #property("deprecated")

Choose a reason for hiding this comment

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

I'm wondering whether we can simply have predefined properties, so they could be simpler:

// #import tidy: *
#deprecated
#contextual

Since we have module, we can also use the properties with "namespace"s to disambiguate:

#tidy.deprecated

Copy link
Member Author

Choose a reason for hiding this comment

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

I would argue this is an implementation detail, so I'd leave it up to the doc parsers and such to come up with a convention.

But we could define some defaults too, to avoid an ecosystem split from people choosing their own conventions.

```
]

Property annotations can be used to document package specific or otherwise important information like deprecation status, visibility or contextuality.

Choose a reason for hiding this comment

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

or otherwise important information

what does otherwise mean?

Copy link
Member Author

Choose a reason for hiding this comment

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

Something that is not package-specific, i.e. deprecation status and visibility, can be considered standard annotations. Whereas something like cetz could define its own annotations for cetz-specific things like how a function interacts with its internal style system.

I assume it's the phrasing that's problematic here so I'll rephrase it.

@tingerrr
Copy link
Member Author

tingerrr commented Jul 10, 2024

There has been some discussion on the Typst discord regarding possible syntax of doc comments.
I'll hold off from merging while this is ongoing and write a TLDR of the conclusions for further discussion.

Thank you for the reviews!

@tingerrr tingerrr force-pushed the tingerrr/zkvkylymtwyl branch from 57b8407 to 2b5dbca Compare July 10, 2024 08:38
@tingerrr
Copy link
Member Author

It seems the discussion about doc comments on the discord is finished, so here's the gist:

Laurenz likewise suggested the use of individual doc comments for parameters.

I think this has a few benefits for type hints (which are a temporary solution) too. We could reuse the return type syntax. This reduces the amount of custom syntax required for doc comments. The return type syntax is also much easier to parse, it only consists of a very specific line prefix and is only expected at the end of a doc comment block (save for properties). Placing the type hint on its own line also has the benefit of making migration easier.

I propose we define parameter doc comments like so:

/// Only examples, descriptions and properties here now.
///
/// -> any
#let func(
  /// The magic number.
  /// -> int | float
  arg: 42,
) = { ... }

Laurenz also suggested using regular comments, e.g. // for documentation, to avoid adding too many tokens (as annotations for warning suppression and similar features also add a new comment-like tokens). I think the concern about those is valid, the more tokens we have, the more complicated it is for a novice to read and understand.

Regardless, I think we should keep triple forward slashes for now and leave removing them to a migration process later down the line. In the end, these are still valid comments and don't conflict with other syntax and migration is very simple.

Furthermore, I would like to restrict the properties and return type annotations to simply be the last part of any doc comment, both being optional, but well-ordered. No empty lines between or after them. This simplifies parsing and makes it easier to read and write.

All in all, we would arrive at something like this:

/// Function description
///
/// -> any
/// #property(visibility: "public")
#let func(
  /// Parameter description.
  ///
  /// -> selector
  arg,
)

Things that are left open:

  • Define some standard properties and, as @Myriad-Dreamin suggested, shorthands for them.
    • See my comment further up on them and tell me what you think should be standard annotations. I think a good start would be:
      • deprecation status with supplemental information (since which version, hint/suggestion what to do instead)
      • visibility (public, private)
      • stability (stable, unstable) (maybe with supplemental information)
      • contextuality (maybe with supplemental information)
  • Either strictly define how cross-references work or leave it up to the implementation.
    • I have left some suggestions further up and think we should simply use the @ref[supplement] syntax. We just need to make sure all implementations agree on which label refers to which function for proper cross-linking.

cc: @Mc-Zen, @Myriad-Dreamin

We're in no hurry, so feel free to take your time with this.

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.

5 participants