-
Notifications
You must be signed in to change notification settings - Fork 750
docs templates: document the label
function and color labels better
#7780
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
base: main
Are you sure you want to change the base?
Conversation
I originally meant to make a smaller change, but I ended up reordering the discussion of different topics, resulting in a large diff. Fixes jj-vcs#7778
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.
LGTM
`label(label, content)` function. For example, | ||
|
||
```sh | ||
jj op log -T '"ID: " ++ self.id().short().substr(0, 1) ++ label("op_log operation id short", "<redacted>")' |
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: perhaps, the latter label()
should be label("id short")
or label("id short substr")
.
It's also good to use practical example such as label(if(current_operation, "current_operation"))
.
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!
perhaps, the latter label() should be label("id short") or label("id short substr").
Do you have any thoughts on how a user could discover the fact that "id short" is sufficient here? Or do you think we don't need to explain it?
If your preference is simply to replace the label with "id short" with no further explanation, I can live with that.
It's also good to use practical example such as label(if(current_operation, "current_operation")).
I had this thought too, but a simple enough example that felt complete didn't come to me.
For both issues, linking to the built-in colors.toml
might help. I kept avoiding it since it is linked indirectly via the "customizing colors" link, and because I was wondering whether to explain things comprehensively, I'd need to insert excerpts from there that would make everything longer. But maybe it makes sense to add it after all in some way.
So, I think both of your points are good, but I don't have an immediate idea for implementing them nicely. I'd be more than OK with postponing them to a potential follow-up.
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 don't have a good readable explanation, and I would just suggest --color=debug
to understand the behavior. It's also not so obvious that self
doesn't contribute to auto-labeling, substr
is labeled, etc.
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 thinking more about explaining the fact that id short
gets the same colors as op_log operation id short
.
In any case, unless I think of something else or you have more thoughts, I guess I'll just replace it with "id short" and perhaps people with questions will ask.
I originally meant to make a smaller change, but I ended up reordering the
discussion of different topics, resulting in a large diff.
Fixes #7778