Skip to content
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

KEP template: add guidance for line breaks #5085

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pohly
Copy link
Contributor

@pohly pohly commented Jan 24, 2025

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.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory labels Jan 24, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: pohly
Once this PR has been reviewed and has the lgtm label, please assign johnbelamaric for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

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

@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jan 24, 2025
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.
@pohly pohly force-pushed the kep-template-line-wrapping branch from d00bd06 to 1d7c177 Compare January 24, 2025 11:53
@@ -44,6 +44,10 @@ When editing KEPS, aim for tightly-scoped, single-topic PRs to keep discussions
focused. If you disagree with what is already in a document, open a new PR
with suggested changes.

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.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@kikisdeliveryservice
Copy link
Member

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

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 7, 2025
@kikisdeliveryservice kikisdeliveryservice self-assigned this Feb 7, 2025
@kikisdeliveryservice kikisdeliveryservice added kind/template Categorizes changes to the KEP template and removed kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory labels Feb 20, 2025
@kikisdeliveryservice kikisdeliveryservice added this to the v1.34 milestone Feb 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/template Categorizes changes to the KEP template size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants