Wrap long lines in comments in Config.md #4951
Conversation
stefanhaller
force-pushed
the
wrap-comments-in-config.md
branch
from
f2b1319 to
e1b4d86
Compare
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesFootnotes
|
|
@rayluo FYI |
schema/config.json
Outdated
| "replace": { | ||
| "type": "string", | ||
| "description": "Replace directive. E.g. for 'feature/AB-123' to start the commit message with 'AB-123 ' use \"[$1] \"", | ||
| "description": "Replace directive. E.g. for 'feature/AB-123' to start the commit message\nwith 'AB-123 ' use \"[$1] \"", |
There was a problem hiding this comment.
I thought the changes would include only splitting long lines, like you did for Config.md and user_config.go in the current PR.
Where will the content inside this config.json be used? Normally, we would not hardcode \n inside raw data. How to wrap those raw data is a job for the component who consumes the raw data; if it can't render the data with proper wrapping, so be it. Just my 2 cents.
There was a problem hiding this comment.
I guess you are right.
Neither Config.md nor config.json are manually edited; they are both auto-generated from user_config.go. The process is that we generate the schema (config.json) first, and the comments in user_config.go are converted to schema descriptions automatically. Then we generate Config.md from the schema. If we want the schema descriptions to not contain linefeeds, but the lines in Config.md to be wrapped, then this means we need to automatically wrap the lines.
Initially I didn't want to do that, because I thought that in some cases this automatic wrapping would look bad, and we'd have to retain manual control over the wrapping; for example in bullet point lists (here's one example). But actually, having them wrapped to the beginning of the line isn't so bad.
So I reverted the first commit, and implemented auto-wrapping of Config.md. I do agree it's better; please have another look.
Where will the content inside this
config.jsonbe used?
It is used by YML language servers to provide auto-completion and documentation on hover. The YML extension in VS Code does this, and it's extremely useful. And it does show that schema descriptions should be paragraphs, not wrapped lines. For example:
In this hover window, lines will be wrapped automatically when the window is too narrow, like so:
There was a problem hiding this comment.
Yes, the new change looks much better.
Since you described the relationship among these files is "user_config.go -> config.json -> Config.md", I have one more optional suggestion. I believe in a version control system (VCS) tenet that "(1) Everything could be stored in VCS; (2) Artifacts that can be derived from #1 shall NOT be stored in VCS". (And I even build a little tool to do that for github pages - but I digress.) Anyway, I mean if you can somehow generate config.json and then config.md dynamically, then you won't need to store them in the code base, and this entire conversation won't need to happen in the first place.
Again, this is an optional suggestion. You don't have to do it, either in this PR or a new PR. Feel free to merge this PR as-is.
There was a problem hiding this comment.
Artifacts that can be derived from #1 shall NOT be stored in VCS
I would normally agree with that, but in this case the artefacts need to be hosted on github: Config.md is used as a poor man's manual substitute (it's linked to from lazygit's Status view), and the schema is referenced by the json schema store so that it can be found automatically by language servers.
Sure, you could argue that it would be better to host them somewhere else, and deploy them to that place as part of making a release, but it's extra work to set this up, and we are lazy. 😄
stefanhaller
force-pushed
the
wrap-comments-in-config.md
branch
from
ae4c55c to
d5e0982
Compare
| startOfLine := 0 | ||
| lastSpaceIdx := -1 | ||
| for i, r := range line { | ||
| // Don't break on "See https://..." lines |
There was a problem hiding this comment.
Nice!
I would say in general all https://... lines shall not be wrapped, regardless of whether it has the "See" or "Please refer to" prefix, though.
There was a problem hiding this comment.
I disagree; if a URL occurs in the middle of a line, I see no reason why we shouldn't break right before it, e.g. here. It's really only the "See https://..." lines that are a problem.
Note that this is not a general-purpose formatter; it is very much tailored to our specific needs, and I expect that we might extend the set of special cases in the future when we run into scenarios that are wrapped badly. We don't currently have "Please refer to" in our documentation, so we don't need a rule for it.
Oh, and btw: this is another reason why it's good to include the generated artefacts in the same commit as the source they are generated from. This allows us to see those badly wrapped cases immediately, rather than only much later when the next release is deployed.
There was a problem hiding this comment.
I disagree; if a URL occurs in the middle of a line, I see no reason why we shouldn't break right before it, e.g. here. It's really only the "See https://..." lines that are a problem.
To clarify, my statement of "https://... lines shall not be wrapped" means do not split one url into two (or more) lines.
If a url occurs in the middle of the line, it is of course OK - and often preferable - to put the url into the next line (such as what your example above shows)
Note that this is not a general-purpose formatter
Fair enough.
There was a problem hiding this comment.
do not split one url into two (or more) lines.
Ah, right. Yes, I agree, and no, we don't do that. We only ever break lines at spaces.
We want to switch to have paragraphs consistently on one line, and auto-wrap them automatically when generating Config.md. The changes to Config.md in this commit are temporary.
stefanhaller
force-pushed
the
wrap-comments-in-config.md
branch
from
d5e0982 to
db9fb21
Compare
2 participants
Since these are displayed in a fenced code block on github, they aren't auto-wrapped on display, so users have to scroll horizontally to read longer paragraphs.
Addresses #800 (comment).