-
Notifications
You must be signed in to change notification settings - Fork 397
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
docs: Move language references to a Reference section #5375
base: main
Are you sure you want to change the base?
Conversation
69193a1
to
3faa39f
Compare
The "Check that MkDocs..." build failure seems caused by this issue: astral-sh/uv#10654 |
3faa39f
to
46ac4a5
Compare
Seems like @ilyagr should review this (others can of course also review). Will you have time to look at this? |
Could you rebase? I'd like to know whether CI will pass now that uv 0.5.20 was released (I'm guessing CI will run after a rebase, hopefully? Though maybe github actions are simply down; I think I saw similar symptoms caused by that once). Since 0.5.19 was the newest version for less than a day, perhaps we can ignore its existence... Update: By the way, the check that uses the latest version of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, splitting config.md
sounds great to me! A reference section also sounds great.
I had a bunch of minor comments. There's also a comment I marked as "Open-ended confusion" about [revsets]
and [templates]
that I'm not sure what to do about.
docs/revset-aliases.md
Outdated
"trunk()" = "dev@origin" | ||
``` | ||
|
||
## `immutable_heads()` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like it would be cryptic to a new reader.
Perhaps something like "immutable_heads()
: changing the set of immutable commits"? Or maybe have a redundant subheading:
## Changing the set of immutable commits
### `immutable_heads()`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was concerned about breaking links if we rephrase it. This anchor is #immutable_heads
, but adding more text would lead to #immutable_heads-changing-the-set-of-immutable-commits
, which would break if we rephrase it to, say, "immutable_heads(): Set of immutable commits".
I didn't see a way to control what anchor MkDocs uses. Ideally, the anchor would be #immutable_heads
but the visible text would be more descriptive.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was also worried a bit, perhaps the "redundant subheading" option might help. If we can't figure out an ideal solution, I think readability is more important than potential changes to heading anchors.
I also don't know a good way to affect MkDocs anchors or add new ones. We could use HTML, problably, but that'd be terrible for reading the docs in the terminal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! I found how to set anchors with just markdown:
### `immutable_heads()`: Set of immutable commits {#immutable_heads}
I've added more descriptive text to both trunk()
and immutable_heads()
.
d56baae
to
b5b9c4f
Compare
mkdocs.yml
Outdated
- 'Revset language': 'revsets.md' | ||
- 'Fileset language': 'filesets.md' | ||
- 'Templating language': 'templates.md' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: just sort them in lexicographical order?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I was thinking to sort them by "importance" but there's only three so it really doesn't matter.
'format_time_range(time_range)' = 'time_range.start() ++ " - " ++ time_range.end()' | ||
``` | ||
You can use relative timestamps by adjusting the [`format_timestamp()` template | ||
alias](template-config.md#format_timestamp). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need links per sub section? I think something like this is good enough:
## Log
...
### Template and style
See [Log template configuration](..) for details .. blah blah
I think related config variables can also be moved there. For example, ui.log-word-wrap
is NOT a template parameter, but it applies to log template output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a request to remove headings like "Relative timestamps", "Author format", etc., and just have one blanket section "Template and style" that links over to the template-config.md page. Do I understand correctly?
@ilyagr suggested keeping these headings so incoming links don't break, like the link to #node-style
that jj log --help
outputs.
But, now that I've found how to add custom anchors to the page, I could make all those anchors go to the relevant blanket section.
### Template and style
[](){#relative-timestamps}[](){#author-format}...
See [Log template configuration](template-config.md) for more information.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a request to remove headings like "Relative timestamps", "Author format", etc., and just have one blanket section "Template and style" that links over to the template-config.md page.
Yes. I don't think we would want to update both config.md and template/revset-config.md when adding new sections.
'commit_timestamp(commit)' = 'commit.author().timestamp()' | ||
``` | ||
The [`commit_timestamp()` template alias](template-config.md#commit_timestamp) | ||
controls whether the author or committer timestamp is displayed in the log. | ||
|
||
### Signature format |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move "Signature format" to template-config.md?
|
||
This configuration section configures the default. These values can use [template | ||
aliases](#template-aliases). | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to briefly describe the overview of the [templates]
section. For example, we had this in the original doc:
You can configure the template used when no `-T` is specified.
- `templates.log` for `jj log`
- `templates.op_log` for `jj op log`
- `templates.show` for `jj show`
[templates]
# Use builtin log template
log = "builtin_log_compact"
# Use builtin op log template
op_log = "builtin_op_log_compact"
# Use builtin show template
show = "builtin_log_detailed"
I also think we should omit detailed section unless there are things to be documented. For example, the ### Show
section is pretty much empty.
b5b9c4f
to
d38baae
Compare
Ah! I found how to set anchors with just markdown
<https://www.mkdocs.org/about/release-notes/#anchors>:
I was initially very excited about this (thanks for finding it!), but now I'm a bit frozen on this point (or, in more professional terms, undecided about what is best to do here 👔). I might write more later, but TLDR:
- This syntax is *not* part of CommonMark, and is not supported by GitHub. It's probably not supported by VS Code, etc. There are some discussions about adding something like this to CommonMark, but they died down years ago, and CommonMark is no longer accepting new features (not sure whether this is a temporary or a permanent state).
- This syntax is supported by MkDocs and [Docusaurus](https://docusaurus.io/docs/markdown-features/toc#heading-ids) (which is my best guess at a CommonMark-based doc builder we could use that has versioning, even though I don't like its heavy reliance on React&JS magic)
- This syntax would look OK if we printed the files unformatted to stdout (like we might be doing with `jj help --topic`).
The HTML `<a id="raw-anchor"></a>` syntax, also explained in Stephen's links,
- should be supported by any HTML-destined Markdown system.
- We'd probably have to find a way to remove it if we print the Markdown to stdout or if we ever want to convert it via Pandoc.
- I haven't tested it, but hopefully MkDocs will recognize these anchors and won't label links to them as broken.
- Not sure about VS Code
I'm not sure whether we should use one of these, or just grit our teeth and only use the anchors given by headings, which everything understand, but would mean that we should try to get the heading structure right the first time (though we shouldn't overdo it either, it's not the end of the world if people have to use their eyes to search through the page, especially if we make it shorter).
Sorry for a sleepy comment with runon sentences.
(I also posted this in a weird way since I had a half-written review that got interrupted by thinking about these points)
|
Just a few brief comments, @ilyagr :
If I type While I don't see it in the CommonMark specification, their "dingus" does in fact render properly too, you can try it here https://spec.commonmark.org/dingus/ In my Windows Terminal, running bash in Ubuntu, links like this are clickable, when I hover over this, it gets underlined, and when I click it, it does open the link in my browser:
A nice thing about Docasaurus is that it's primarily server-side rendered, so I'm not sure which parts you object to, but it's all at build time, not in the browser. Rust's own comment style guidlines for APIs suggest using |
@steveklabnik I meant the "specifying the anchor" syntax, Aside: As a test, here is a link to two ids below: first id, second id. The first one does not work, the second one does You can see the GitHub markdown rendering here failing here for You can also see it below. Here are the targets of the above linksThe source is:
This anchor does not work {#my-custom-heading-id} |
I think the reference pages for the revset, fileset, and templating languages are different concerns from the configuration reference pages.
As suggested by @arxanas, I've moved the language descriptions into a new Reference section.
Then I created new pages
revset-aliases.md
andtemplate-aliases.md
in the Configuration section for the well-known aliases likeimmutable_heads()
,format_timestamp()
, etc.