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 @(auto)docs secondary #2237

Merged
merged 3 commits into from
Sep 15, 2023
Merged

Conversation

frankier
Copy link
Contributor

@frankier frankier commented Aug 29, 2023

This allows @docs to be included multiple times, as long as all but one are marked as secondary. References will then only target the primary @docs. Also implemented for @autodocs.

Closes #1570
Closes #1079

Thanks @manuelbb-upb for initial PR which I used as reference and @mortenpi for feedback which this new PR mostly follows.

TODO:

  • Add tests
  • Warning or an error if a particular docstring is included only as a duplicate

@frankier
Copy link
Contributor Author

I've now excluded secondary docs from from the missing docs test, which should probably be enough in terms of checking. I can add tests if needed, but again I'm not really sure where to add them. Do I add to an existing end-to-end test or create a new unit tests?

@frankier frankier marked this pull request as ready for review August 29, 2023 11:53
@fredrikekre
Copy link
Member

Don't know if they necessarily have to align, but DocumenterCitations uses canonical = false for secondary bibliographies, see https://juliadocs.org/DocumenterCitations.jl/stable/syntax/#noncanonical

@frankier
Copy link
Contributor Author

That's an interesting point of comparison, however, while I somewhat prefer the key/value approach used there, these type of inline @expander arguments seem to be what's used in Documenter.jl itself. We can search/replace the name if there is consensus on something else -- although I would prefer to avoid the previous "duplicate" since it only answers "what" and not "why".

@frankier
Copy link
Contributor Author

This is also related to the new strict=true behaviour.

#2051

I have found some of these double docs in the wild with DemoCards.jl. A good workaround will help people keep strict mode on.

Copy link
Member

@mortenpi mortenpi left a comment

Choose a reason for hiding this comment

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

We can't really do in-block key-value settings here, since the contents of an at-docs block are the methods. So I think the at-tag is the right place for this. And yeah, let's have consistent syntax for at-autodocs too1.

I am not a huge fan of the word "secondary" here and "canonical" sounds better here. And I think being consistent with DocumenterCitations is a good thing. So I suggest we use canonical = false. It should look something like:

```@docs; canonical = false

Note the ; -- I'd say we want this to be a keyword (c.f. doctests that have both positional and keyword argument in the at-tag: https://documenter.juliadocs.org/dev/man/doctests/#Block-level-setup-code).

Note: not a requirement for the PR, but something I have always wanted is a reusable, self-contained function that takes the at-tag string and parses the tag + positional + keyword arguments into a datastructure. Right now, I think every at-block implements their own regex, which is not great in terms of maintaining consistent parsing rules.

Regarding tests:

  • One place where we could should add a "test" case is in docs/src/showcase.md.
  • If you can figure out how to unit test this, that would always be great. But I think it's fine if we just have a case in the standard test/examples/src build.

One last question: what is the missingdocs story right now? I would expect that we still throw a :missingdocs even if you have the docstring included as secondary / canonical = false?

Footnotes

  1. An argument could be made that maybe we should have in-block Secondary = true (or Canonical = false). But I think it's good to be consistent. Plus, we can argue that what is inside the docs block is still information about which docstrings get included, and then the at-tag arguments say how we interpret them.

src/expander_pipeline.jl Outdated Show resolved Hide resolved
@frankier
Copy link
Contributor Author

frankier commented Sep 5, 2023

Okay I've done this now. The keyword arguments are a bit more verbose/line-noisy, but quite a lot more futureproof since we could easily end up with many more options/wanting to change defaults so quite happy with that.

One last question: what is the missingdocs story right now? I would expect that we still throw a :missingdocs even if you have the docstring included as secondary / canonical = false?

That's right.

This allows @docs to be included multiple times, as long as all but one
are marked as noncanonical. References will then only target the
canonical @docs. Also implemented for @autodocs.
@frankier
Copy link
Contributor Author

Rebased

Copy link
Member

@mortenpi mortenpi left a comment

Choose a reason for hiding this comment

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

This is great, and seems to work advertised. Thank you @frankier!

@mortenpi mortenpi merged commit 2bfc172 into JuliaDocs:master Sep 15, 2023
20 of 21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: duplicatable docstrings
3 participants