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

Allow for duplicate docstrings in @docs and @autodocs, issue #1079 #1570

Closed
wants to merge 5 commits into from

Conversation

manuelbb-upb
Copy link

@manuelbb-upb manuelbb-upb commented Apr 28, 2021

This is a proposal on how to fix #1079.
By default, the standard behavior is maintained, that is, duplicate docstrings are not printed.
However, @docs and @autodocs now allow for passing an id, i.e.

 ```@docs id1
some_function
```

The id will be Base64-encoded and used to compare objects and to define URLs; the default id is the empty string.
Docstrings from blocks with differing ids will be printed and have differing anchors.

Additionally, @index blocks will refer only to docstrings from blocks with the empty id string by default.
This behavior can be changed by providing the keyword include_ids:

```@index
include_ids = ["", "id1"]
```

I have not yet adapted the documentation for the proposed syntax.

@manuelbb-upb manuelbb-upb marked this pull request as draft April 28, 2021 12:00
function Object(b::Binding, signature::Type)
id_str :: AbstractString

function Object(b::Binding, signature::Type, i_s :: AbstractString = "")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
function Object(b::Binding, signature::Type, i_s :: AbstractString = "")
function Object(b::Binding, signature::Type, i_s::AbstractString = "")

@@ -273,6 +273,7 @@ end
# -----

function Selectors.runner(::Type{DocsBlocks}, x, page, doc)
id_str = match(r"^@docs[ ]?(.*)$", first(split(x.language, ';', limit = 2)))[1]
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps the syntax should be

```@docs id="something"
```

instead? I think that pattern is used elsewhere for e.g. doctest filters etc.

Copy link
Member

Choose a reason for hiding this comment

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

Also, do you ever care about the id or just that it is unique?

Choose a reason for hiding this comment

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

Maybe the current approach follows the idea that is used in example blocks? there the id/name for continued examples is also just added with a space.

Copy link
Member

Choose a reason for hiding this comment

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

Yea that's true.

Copy link
Author

Choose a reason for hiding this comment

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

Also, do you ever care about the id or just that it is unique?

If I remember correctly, I need the id_str for the filtering mechanism in @index blocks.
This filtering should allow to

  • exclude docstrings with ids that are not empty from the listing
  • and to list only a subset of docstrings.

Copy link
Member

Choose a reason for hiding this comment

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

is it ever useful to have index with duplicates though? Perhaps there should be a canonical @docs block or something?

Copy link
Author

Choose a reason for hiding this comment

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

is it ever useful to have index with duplicates though?

I guess not, and I am not sure myself anymore what I wanted this feature for.
I think it was primarily meant for exclusion of docstring subsets, but most of the use cases that come to mind can also be achieved with the Pages kw.
The only (exotic) example I can think of is a single documentation page that has subsections (possibly containing duplicate docstrings in @docs blocks) and an @index is meant to be created for each subsection.

@fingolfin
Copy link
Contributor

So, does it seem like this has a chance to be merged? In this case, I guess the main TODO is to adjust the documentation? Or is there any other major blocker?

@fredrikekre
Copy link
Member

Yea, I am just not a big fan of the API here. If a @docs block is marked with an identifier, does that mean that all docstrings in that block are duplicates? I am guessing that the most common use case is when you would like to splice in a single docstring in a "manual page" that also should exist on a "reference page"? So in those cases there would probably be quite few docstrings, like

Some text describing `foo`:

```@docs duplicates
foo
```

etc...

@manuelbb-upb
Copy link
Author

Yes, that's the main use case (for me personally, but also from what I gather at #1079).

From how it looks in this line indeed every single docstring in a single @docs block is marked with the id.
But for the main use case that's fine as there will usually only be a single docstring per block.

As an alternative to the current syntax, maybe a new block type (e.g. @docstring) could be used?
Instead of user assigned identifiers, we could generate a random uuid4() in the background and mention in the docs, that @docstring content will not be referenced in an @index.

@fredrikekre
Copy link
Member

Yea, I have definitely wanted that myself too at some point.

I did not try this out, but one idea would be to only actually put the id if there are duplicates detected in the block.

@manuelbb-upb
Copy link
Author

I did not try this out, but one idea would be to only actually put the id if there are duplicates detected in the block.

The problem I see with this approach (i.e., taking the user out of the loop) again is the behavior of @index.
Suppose you had a manual page

Some text describing `foo`:

```@docs
foo
```

etc...

and a reference page

Module API Page

```@autodocs
…
```

Without further hints from the user, @index would then have duplicates and list both foo two times.
If we disallow duplicates in @index, we would somehow have to decide, which docstring to point to.

@fredrikekre
Copy link
Member

fredrikekre commented Feb 3, 2022

If we disallow duplicates in @index, we would somehow have to decide, which docstring to point to.

Yea, my idea then would be that the one without an id would be the canonical one where all links would point to, and the duplicated one. Is it useful to be able to link to both of them individually?

@mortenpi
Copy link
Member

mortenpi commented Apr 27, 2022

@manuelbb-upb: are you still interested on working on this?

I agree with @fredrikekre's feedback, but I would go further and say that I am not sure that the whole unique id thing is actually necessary. I would propose the following design as an alternative:

  • Allow an at-docs block to be marked "duplicate" (e.g. with just @docs duplicate). Docstrings from such a block are spliced in, but at-ref links can not point to these. There should probably be a warning or an error if a particular docstring is included only as a duplicate.
  • By default, duplicate docstrings are not included in the index, but there could be an option to include them as well (e.g. if you're doing per-page indices). I am not sure we really need any more fine-grained control over this.
  • Each duplicate docstring would still need an internal HTML id=... (e.g. when linked to by an at-index), but we can generate this automatically.

Things such as more fine grained filtering and marking individual docstrings as duplicate (as opposed to whole blocks) are things could be implemented later if the demand arises.

@manuelbb-upb
Copy link
Author

@mortenpi As it is not that urgent a feature request, I should be able to find some time in the next few weeks to adapt the proposed code changes to your suggestions.
If I remember correctly, I actually first tried automatic UUIDs and then I might have overdone it a bit 😁

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.

Feature request: duplicatable docstrings
5 participants