KEP template: move guidance for line breaks

[pohly]

The last three new KEPs that I started to review all didn't use line-wrapping, including one from an experienced Kubernetes contributor. Apparently we have to put the existing guidance on line breaks into a more prominent spot.

The problem with one line per paragraph is that in a review, you can only comment on that one line. That's bad for several reasons:

• If anything at all gets changed in that paragraph, like a single character change to fix a typo, all comments are treated as obsolete by GitHub. They completely disappear from the file diff view.
• It's harder to see which part of the paragraph a comment is attached to.
• Editing a suggestion is harder because a long text has to fit into a small text box.

Then there's the diff between different revisions. Each diff is much larger than it needs to be with one line per paragraph. Word highlighting helps, but only a little. See https://sembr.org/

[pohly]

And to demonstrate semantic line breaks, this last sentence should be its own line :-)

https://github.com/kubernetes/enhancements/compare/d00bd0651950b0bda87e2c4cab9e0e5268325091..1d7c1772d320cd0b00dcbd275d400a71f4f00eb5

[dom4ha]

Having style guidelines would be great (I completely haven't payed attention to it).
Is there a recommended line width or just semantic line breaks recommendation? Following just semantic, I'd format this entry like this:

Wrap long lines instead of using one line per paragraph.
When updating, change as few lines as possible. This helps with anchoring review comments.
Semantic line breaks (https://sembr.org/) are useful.

[pohly]

You are right, strict sembr would not reflow the first two sentences.

I think just raising awareness should be enough. I don't want to be too pedantic and in particular, don't want reviewers to spend time on enforcing some kind of style guide.

[thockin]

If we had a bot that could review for style, would you want it to suggest sembr ?

IMO an ideal flow would be a bot saying "click this link and I will post a diff to convert this to sembr".

Agree we don't want to enforce a style guide, per se, but this should be strongly encouraged. We should also make it clear that the comments are to be deleted in each section, as it is filled out :)

[pohly]

If we had a bot, I would make it post the text above.

But I don't think we need a bot, just a documented agreement that a reviewer can link to when encountering a new KEP which doesn't wrap lines.

[johnbelamaric]

First I have seen sembr. It seems like "carrying a logical idea out to its illogical conclusion", IMO. But, yes to line breaks in general...I just gwap in vi...

[thockin]

V, select text, gq is my habit.

I went and read some of the sembr examples and honestly, it feels awkward at first but I think I can get used to it if it makes diffs nicer.

[pohly]

Same here. I am slowly starting to adopt it a bit more. The good thing is that it doesn't need to be followed religiously.

[kikisdeliveryservice]

This looks good, but as noted in other PR, let's just hold until after enhancements freeze.

/hold

[pohly]

@kikisdeliveryservice: can we merge this now?

[BenTheElder]

The last three new KEPs that I started to review all didn't use line-wrapping, including one from an experienced Kubernetes contributor. Apparently we need better guidance.

It's normal not to line-wrap markdown, it will be reflowed on rendering.

If we're going to set formatting expectations, are we going to provide tooling for it?

Who discussed and agreed that this is recommended?
Do we have such guidance elsewhere in the project? (e.g. docs)

[BenTheElder]

I think just raising awareness should be enough. I don't want to be too pedantic and in particular, don't want reviewers to spend time on enforcing some kind of style guide.

The KEP template is already enormous, to the point that people actively avoid KEPs, including experienced contributors, if we're not enforcing this, and it's just a suggestion ... I'm not sure we should make the KEP template even more intimidating.

[BenTheElder]

Not line wrapping also means that the client fully controls the line length, inserting hard line breaks will create disjointed lines if my client doesn't match the width selected. Most people have a harder time reading lines of constantly varying length (which is why text justification is so popular and pretty much standard in books).

Github by default fills to the browser width, so you can control this when reviewing by adjusting the width of your review window.

[pohly]

@BenTheElder: you are focusing on viewing. That is fine and not the reason why I suggested this.

The problems with one line per paragraphs is that in a review, you can only comment on that one line. That's bad for several reasons:

• If anything at all gets changed in that paragraph, like a single character change to fix a typo, all comments are treated as obsolete by GitHub. They completely disappear from the file diff view.
• It's harder to see which part of the paragraph a comment is attached to.
• Editing a suggestion is harder because a long text has to fit into a small text box.

Then there's the diff between different revisions. Each diff is much larger than it needs to be with one line per paragraph. Word highlighting helps, but only a little. See https://sembr.org/

[pohly]

Do we have such guidance elsewhere in the project? (e.g. docs)

Yes: https://kubernetes.io/docs/contribute/style/style-guide/#line-breaks

"Manually wrap paragraphs in the Markdown source when appropriate. Since the git tool and the GitHub website generate file diffs on a line-by-line basis, manually wrapping long lines helps the reviewers ..."

I'm not asking for hard rules here either, just using common sense. sembr can be useful, but I don't care whether an author uses it.

I thought it was obvious that line wrapping is useful. Apparently not, so let's hear from others who do a lot of reviews of KEPS whether we should add some guidance to KEP authors.

/cc @thockin @liggitt @soltysh @johnbelamaric

[thockin]

As someone who reads and comments on a lot of KEPs, I 100% endorse linewrapping aggressively for the original reasons. GitHub (and diff, in general) handles long lines very poorly WRT merging. Keeping track of outstanding comments in a KEP is one of the most tedious and time-consuming things I have to do.

I personally use 80 characters for comments and markdown, and sometimes that is too much. I am committed to trying to learn to use semantic line breaks.

[liggitt]

Big +1 on line breaks instead of one line per paragraph.

Also big +1 on this being a nice-to-have, not a hard requirement or something we'd enforce with a presubmit check or anything.

There's not a great spot to hang this advice in the KEP template... where this PR drops it in feels disconnected from the other procedural advice in the comment.

[BenTheElder]

The problems with one line per paragraphs is that in a review, you can only comment on that one line. That's bad for several reasons:

Great point. Thanks.

[tengqm]

We the k8s docs team uses sembr style although we were not aware of this term.
Writing a paragraph in one line does hinder reviews and change tracking;
strict wrapping at fixed width doesn't make a lot of senses either
(every time you add a single word, you may need to reformat the paragraph again,
that means your change overflows to many lines).
When getting used to sembr style, contributors can quickly get the ideas behind it
and become supportive.

[pohly]

@tengqm: do you have guidelines that we could link to here?

Telling contributors they need to manually fix formatting is pretty much always terrible, that's why we have gofmt.

I don't think there is any way how text can be formatted automatically the way Go is. @BenTheElder, I think you are looking for a technical solution to a problem that ultimately boils down to "use common sense".

Raising awareness that "one line per paragraph" is undesirable is all that I am looking for. I had to leave that comment three times this cycle when reading new KEPs (not counting the latest one, which wasn't reflown by mistake). Reflowing after the PR has been opened is too late, others may have already started to comment.

I don't care too much where we drop that hint. Given that everyone starts with the template and there are other pieces of advice nearby like "remove comments" or "use UNRESOLVED", it doesn't seem entirely out-of-place. I agree with @thockin that putting it into a separate doc on how to write KEPs is probably going to be missed.

[johnbelamaric]

I agree that it makes sense to break to make diffs / review more managable. However, there's no such thing as unrendered text, unless you mean in-memory. You're still reading a rendering of the text, which is at the width of your client, be it a text editor or a browser.

Fair, of course. But specifically, say, looking at the textual representation in GH or vi, not wrapping is rough to follow. The goal of MD was for it to be readable with and without an advanced renderer.

I'm sorry but I'm not memorizing a spec for line breaks, there's way too much complexity to keep up with already.

Yeah, I am with you here.

[BenTheElder]

I don't think there is any way how text can be formatted automatically the way Go is. @BenTheElder, I think you are looking for a technical solution to a problem that ultimately boils down to "use common sense".

1. The problem is when "common" sense isn't actually common / agreed, and we leave contributors in a bikeshed tug of war.
   Telling contributors to go read and attempt to follow an external doc is marginally better but still leaves room for "no not like that".
   We already ask a lot of anyone looking to contribute a feature.

2. I'm not convinced that markdown cannot be auto-formatted in general,
   if we want to ask for certain formatting, sembr however is slightly complex.
   For example "wrap at 80 characters, do not break words" which sembr recommends, is pretty simple to implement.
   "Wrap after sentence termination and other punctuation" is slightly trickier (you'll probably want to exclude abbreviations).
   A quick search turned up https://github.com/remarkjs/remark to lint + auto-format,
   though I don't think any of the existing plugins follow sembr.

Also, I think it says something that this PR doesn't follow sembr, so we're already deviating on what is recommended / "common".

From the sembr spec:

A semantic line break MUST occur after a sentence, as punctuated by a period (.), exclamation mark (!), or question mark (?).

This PR does not wrap after each sentence.

+Wrap long lines instead of using one line per paragraph. When updating, change
+as few lines as possible. This helps with anchoring review comments.
+Semantic line breaks (https://sembr.org/) are useful.
+

It would have to be:

+Wrap long lines instead of using one line per paragraph. 
+When updating, change as few lines as possible. 
+This helps with anchoring review comments.
+Semantic line breaks (https://sembr.org/) are useful.
+

Which doesn't seem easier to read.
Narrow lines reduce readability: https://baymard.com/blog/line-length-readability

(This comment attempts to follow sembr)

[johnbelamaric]

Which doesn't seem easier to read.

Agreed. My "common sense" tells me a line break after every period is not readable.

[pohly]

The problem is when "common" sense isn't actually common / agreed, and we leave contributors in a bikeshed tug of war.

You are assuming that reviewers will complain about "bad" formatting. At least I won't. The only thing that I complain about is no line breaks at all. Have a little trust in your fellow developers? 😅

This PR does not follow sembr to the letter because that's not what I am asking for. Awareness of how line breaks affect reviews and diffs is all I am looking for.

[thockin]

I almost never complain about line-length anymore because it destroys github comments. That said, when I see a one-line-per-paragraph, I hate reviewing it.

I'm in the "wrap at 80 by default" camp, which strikes an overall good balance - diffs are somewhat constrained but not so much that they impact readability.

I actusally like Ben's reformatted version of this PR. I have 50 other gripes of things people do that make reviews hard, this one is not in my top 3, but I still agree with it.

The only real answer is automation which IMMEDIATELY reponds to any push with "It looks like you could use help with linebreaks. Here's a diff you can accept which re-flows your document, which can make life easier for your reviewers

[pohly]

The only real answer is automation which IMMEDIATELY reponds to any push with "It looks like you could use help with linebreaks. Here's a diff you can accept which re-flows your document, which can make life easier for your reviewers

At which length would we emit that warning? And can it be a warning or does it have to be a hard job failure in pull-enhancements-verify? Neither is ideal: if it's not a hard job failure, no-one is going to see it. If it becomes a hard limit, we can never deviate from it even when it might make sense to have a long line. Worse, updates of some existing PRs might have to reflow everything.

[soltysh]

I'm similarly to Tim in the 80 char line break. I have 2 vertical lines in my editor one is at 80 which is ideal, and the other at 120. If we could make the tool which would try to strike a reasonable balance and allow us to configure a range, for example 80-100, not to be super strict and resolve certain problems that were called out earlier in the comments I think that could be a reasonable balance. But I feel like I'm leaning towards a tool, because that's the simplest to enforce.

[BenTheElder]

You are assuming that reviewers will complain about "bad" formatting. At least I won't. The only thing that I complain about is no line breaks at all. Have a little trust in your fellow developers? 😅

This PR does not follow sembr to the letter because that's not what I am asking for. Awareness of how line breaks affect reviews and diffs is all I am looking for.

Well .... you might not, but as you can see here, people will bikeshed the details, and we're pointing to a very specific guide that we're not following so ...

If we just want to suggest "using line breaks is a good idea, avoid overly long lines because it makes review comments hard", let's say that? (and drop the link to the spec we don't intend to follow)

---

[...] But I feel like I'm leaning towards a tool, because that's the simplest to enforce.

We don't necessarily have to even enforce anything.

That could be really annoying for updating older KEPs for example, but if we want to say "here's how you should format", providing a tool is better.

That way we have agreed in one place, and any disagreements are worked out in the tool(ing) instead of individual PRs, and the fix is less manual.

I care less about the specific format than avoiding future arbitrary formatting tangents.

[kikisdeliveryservice]

If we just want to suggest "using line breaks is a good idea, avoid overly long lines because it makes review comments hard", let's say that? (and drop the link to the spec we don't intend to follow)

Taking a step back and looking at the actual template, we do actually already say that here:
https://github.com/kubernetes/enhancements/blob/master/keps/NNNN-kep-template/README.md?plain=1#L168

Both in this section and below, follow the guidelines of the [documentation
style guide]. In particular, wrap lines to a reasonable length, to make it
easier for reviewers to cite specific portions, and to minimize diff churn on updates.

And point to the kubernetes style guide which comes with it's own line length guidance.

I'm not sure of the utility of adding another mention about line breaks and pointing to an external resource. Perhaps this section is in the wrong place, we can certainly move it (maybe before the summary section to make it clear it applies to all the sections) but it indeed already exists.

[pohly]

Thanks @kikisdeliveryservice for pointing this out! I suspect that everyone involved in the discussion so far (definitely me!) missed that this already exists.

+1 for moving that to the header, without any changes, and dropping what I added.

[pohly]

I've updated this PR such that it merely moves the guidance that already existed in the "summary" section into the "Note" at the top. No new policy, no reference to sembr, just a bit more visibility...

Perhaps we can merge it now before people start writing new KEPs for 1.34?

[soltysh]

Suggested change

	Follow the guidelines of the [documentation
	(empty line ;-) )
	Ensure the entire document complies with the [documentation

[pohly]

I actually saw it as part of the same Note paragraph. But as it's not getting rendered it doesn't make a difference - whatever reads better in the code.

But as we are now discussing it: what if we move this note out of the comment block?

Then if some new contributor starts a new KEP and completely skips over this advice, it will stand out in the rendered README.md. Perhaps the new contributor then even notices themselves that they missed something?

[BenTheElder]

Adding more text you have to delete sounds annoying and it seems unlikely that the new contributor is looking at rendered vs not-rendered markdown differently. (They probably won't see it rendered at all)

For detection ... well they either followed the guidance or didn't ...

[pohly]

Could be.

Anyway, pushed with an additional empty line.

[BenTheElder]

/lgtm

[pohly]

/assign @kikisdeliveryservice

For approval.

[thockin]

ping for approval? @kikisdeliveryservice

[dims]

/approve
/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, pohly

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• keps/OWNERS [dims]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment