Doesn’t this also show that we are adjusting our guidelines to compensate for GitHub’s poor UX?
Btw, you might have already tried this, but there’s Hide Whitespace Changes in the UX, see: Committing and reviewing changes to your project in GitHub Desktop - GitHub Enterprise Server 3.16 Docs.
How so? Reflow requires you to change every line until the end of the paragraph. It is affecting both GitHub UI and git blame, which definitely can’t be accused of being influenced by GitHub’s poor UX.
What impact will putting an entire paragraph on a line have on git blame?
With one line per paragraph, there would indeed be no difference. But that’s not the only way, and not even the way I originally mentioned.
If git blame is a concern, then it seems one line per paragraph is not a good solution as it can make that usage worse.
If git blame is not a concern, then it seems GitHub’s UX is the culprit. It is too bad there is no git diff --color-words for GitHub.
Am I understanding correctly?
I was confused by an earlier suggestion of “Hide Whitespace Changes”, because it doesn’t help, but various modes of git diff --word-diff can indeed handle my example. Technically yes, GitHub UX could be improved to add this option to the existing option of ignoring whitespace changes. But unlike other problems we have with GitHub, I’m not sure other systems for code review we could’ve migrated to have this feature. It’s been a while, but I don’t remember Phabricator being able to do anything but hide whitespace changes.
Strong -1 from me.
I often view files in browser tabs where resizing the browser window to reflow text is awkward and obviously affects other tabs. Having more content on single lines also increases the number of iterations of git blame required to identify where some text originated.
The slow to non-existent improvements to GitHub UI is not, in my opinion, justification for making other experiences worse.
+1 for changing this.
I don’t have strong preferences for how, imo even just raising the column limit would make this a lot easier to deal with in the case of e.g. long code-blocks. Though I find the “one sentence per line” principle seems the most convenient.
I don’t think that putting the entire blame on Github’s UX here is meaningful: completely separately, I (as a contributor of many release notes in the past few weeks) have constantly had to reflow a bunch of lines just because I inserted a few words at the top.
And even from a reviewer perspective, I think that going with a setting that is mostly likely to be compatible with various platforms (including Github) is to the benefit of everyone. Even if you can work around this with special settings and what not, the average reviewer likely doesn’t know about these (as we can also see in this thread)
To clarify: For me, the GitHub UX discussion does not indicate, for example, that migrating to GitHub PRs was a bad choice. Instead, it suggests where the ideal solution to the problem would be implemented. How to get it implemented in GitHub or an extension is also a problem.
There’s several cases where it’s more convenient to keep things as a single line, I usually do this for release notes / itemized lists. Code blocks in .rst files require indentation so pretty much all of those go beyond 80 characters. There’s benefit to being loose here, but I’d prefer that standard text paragraphs remain easily readable no matter if I’m reading it through the web-UI, vim, or cat.
This RFC changed a bit since I left my last comment, so adding some more details.
I continue to be strongly in favor of one line per sentence; as someone who writes and reviews a lot of these files, that’s truly the best option.
Most critically, it is a rule everyone can follow. An 80 column limit is not tractable; we already see that people don’t follow that guideline in all sorts of ways. Sometime it’s just ignored because of word wrap in editors, sometimes it’s ignored because of external links, sometimes it’s ignored because of code blocks, etc.
It makes reviewing diffs possible. When people re-flow to 80 col limits, reviewers continually have to ask “what actually changed here?” and that’s a needless burden on everyone.
It’s something we can automatically check as part of precommit CI. It’s relatively easy to know what breaks a sentence, so we can actually enforce this rule. We cannot enforce things like one paragraph per line because it’s not easy to know what a “paragraph” actually is in documentation. e.g., if I have some text, a code block that’s an inline quote, and some more text… I may want that to be a single paragraph, but how does a tool know that? The same is true for splitting by punctuation, should we split on ; or :, what about -- vs -, etc? These edge cases will lead to inconsistency in practice.
I developed a script to make it easier to use git diff and options like --color-words when viewing GitHub PRs. For example, for PR #171453:
$ cd llvm-project.git
$ gh-pr-diff --color-words 171453
--color-words has been around for many years and makes it much easier to read diffs for reflowed text.
I understand this script does not address every concern raised in this RFC. It also requires access to a terminal while reviewing PRs. For those reasons, perhaps the community will decide to accept this RFC anyway.
Regardless, I expect the script will help with cases the RFC does not address, such as reflowed C++ source code comments.
If you think you or others will use the script, please say so in the PR, even if you do not wish to be a reviewer. If there is no interest, it will not land.
Is there a clang-format-like tool that would automatically format per-sentence?
MLIR is all in Markdown and I’m used to mdformat -wrap 80, I could do with a new per-sentence guideline assuming there is a tool to handle it automatically for me.
I voted for the last option (split by parts of sentences by punctuation), but that’s not my ideal preference (to avoid “long lines”, allow split by parts of sentences but not necessarily by punctuation).
I think the discussion is missing the idea of discretion and the possibility of suggested points for breaking “long lines”. Some guidance on a soft column limit buffer would be good.
The main problem with the status quo is reflow, which (in my experience) can be generally avoided if we don’t encourage uniform “density”.
Yes, Prettier has a Prose Wrap option which can be set to never to un-wrap each block of prose into one line.
Thanks @JDevlieghere : I tried this option, however it does not seem to operate per sentence but per “block” (paragraph?).
Here is an example of applying npx prettier . --write --prose-wrap=never to our doc:
You’re right, I misremembered it working by sentence, but it’s by block. I’m surprised the option doesn’t exit, I can’t imagine it being harder than doing the wrapping, but I also know prettier is very opinionated so maybe this is intentional.
@project-council Can we call consensus on this RFC? Or put it on the agenda of the next project council meeting, if that is deemed necessary to call consensus?
I’ve added it to the project council agenda for our next meeting, thank you for the ping!
