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

Edit "About this guide" for semantic line feeds #1243

Merged
merged 1 commit into from
Oct 28, 2021
Merged

Edit "About this guide" for semantic line feeds #1243

merged 1 commit into from
Oct 28, 2021

Conversation

pierwill
Copy link
Member

This is a small PR to demonstrate what work toward #1241 looks like.

Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

This LGTM, but I'd like to run this by @spastorino or @JohnTitor before making a large change - do you think #1241 is a good idea?

@pierwill
Copy link
Member Author

Using #1132 for discussion per @JohnTitor.

@JohnTitor
Copy link
Member

The idea itself sounds great to me. But Ralf said:

The change doesn't have to happen all at once; when existing text is changed one could encourage also changing it to the new format.

this makes sense to me, otherwise, the transition would be diff-noisy. However, it raises another concern, "do we have to ask contributors to follow that change each time?". To reduce our work, we could implement a style checker e.g. getting git-diff and checking if each line ends with . or ,.
I'm not sure which one is better so I'd like to see your thoughts.

@pierwill
Copy link
Member Author

The change doesn't have to happen all at once; when existing text is changed one could encourage also changing it to the new format.

I agree!

I'm not sure about automated checking. Lines need not end in a , or .; : works as well. So does no punctuation for certain long sentences. Sometimes splitting at "and" or "but" or "therefore" makes sense.

If they're too much noise, we don't have to do PRs like this one. However, doing this kind of formatting often helps catch typos, confusing sentences, etc.

@pierwill
Copy link
Member Author

do we have to ask contributors to follow that change each time?

This could be case-by-case. For a small change, I'd say no. But if a contributor is doing more substantial editing or writing, I think it'd be okay to ask they follow this style.

@jyn514
Copy link
Member

jyn514 commented Oct 28, 2021

👍 I think this is fine to merge as is and we can add automated checking in a separate PR if necessary.

@jyn514 jyn514 merged commit f3fb373 into rust-lang:master Oct 28, 2021
@pierwill pierwill deleted the semantic-lines-about branch October 28, 2021 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants