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.

Sign up for GitHub

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

Jump to bottom

Remove double spaces after dots in comments #104505

Merged
merged 3 commits into from
Jan 18, 2023
Merged

Remove double spaces after dots in comments #104505

merged 3 commits into from
Jan 18, 2023

Conversation

WaffleLapkin
Copy link
Member

Most of the comments do not have double spaces, so I assume these are typos.

@rustbot
Copy link
Collaborator

rustbot commented Nov 16, 2022

r? @jackh726

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 16, 2022
@rustbot
Copy link
Collaborator

rustbot commented Nov 16, 2022

These commits modify compiler targets.
(See the Target Tier Policy.)

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred in diagnostic error codes

cc @GuillaumeGomez

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Changes rustc_apfloat. rustc_apfloat is currently in limbo and you almost certainly don't want to change it (see #55993).

cc @eddyb

@GuillaumeGomez
Copy link
Member

I wondered why we have some double white space in some places too.

@saethlin
Copy link
Member

It wouldn't hurt to have uniformity, but for some people this is a deliberate stylistic choice. I suspect the majority of these are deliberate, not typos.

@GuillaumeGomez
Copy link
Member

Interesting. It doesn't have an impact on generated documentation so I guess it's for people reading the source code directly?

@WaffleLapkin
Copy link
Member Author

@saethlin a lot of these changes happen in comments with multiple sentences, where only a single double space is present, so I don't think majority is deliberate.

@GuillaumeGomez most of the changes happen in comments, not doc comments, so these are for source code readers anyway :)

@bors
Copy link
Contributor

bors commented Nov 19, 2022

☔ The latest upstream changes (presumably #104600) made this pull request unmergeable. Please resolve the merge conflicts.

@rust-cloud-vms rust-cloud-vms bot force-pushed the no-double-spaces-in-comments branch from fa05ac6 to d2b4eaf Compare November 21, 2022 09:51
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-cloud-vms rust-cloud-vms bot force-pushed the no-double-spaces-in-comments branch from d2b4eaf to b6f8f0a Compare November 21, 2022 10:07
@jackh726
Copy link
Member

So...I'm inclined to think that we shouldn't merge this.

First, I've learned that some people have been taught to use two spaces between sentences instead of one. So it's very much possible that many of these are intentional.

Second, the difference between one and two spaces visually doesn't make much difference. It also doesn't really hinder the effectiveness of the comments in any way.

Third, given point one that these might be intentional, we don't have any style guides or rules for one space vs two. And moreso, we don't have a way to keep new comments from having two spaces between sentences.

Given all the above points and that this is kind of touches a lot of files (and I don't think historically we have really sought pull requests for simple stylistic changes), this doesn't really seem like something worth merging. But I'm curious on others' opinions.

@WaffleLapkin
Copy link
Member Author

Second, the difference between one and two spaces visually doesn't make much difference.

I don't agree, for me it makes a big difference, that's why I've made this pr in in the first place — my eyes caught on the double spaces every time I saw them.

And moreso, we don't have a way to keep new comments from having two spaces between sentences.

It was proposed that we can grep for something like \w\. \w in tidy.

I do not really have anything to say to other points though. I do think having a stylistic choice here would be nice. But if it's only important for me, then leaving this as is okay.

@jackh726
Copy link
Member

I'll just cc @rust-lang/compiler for other thoughts then

@oli-obk
Copy link
Contributor

oli-obk commented Nov 22, 2022

As long as tidy checks it, and in a way that submodules are ignored, this seems fine to me

@bors
Copy link
Contributor

bors commented Nov 25, 2022

☔ The latest upstream changes (presumably #103693) made this pull request unmergeable. Please resolve the merge conflicts.

eddyb
eddyb suggested changes Nov 28, 2022
View reviewed changes
compiler/rustc_apfloat/src/ieee.rs Outdated Show resolved Hide resolved
@jackh726
Copy link
Member

@WaffleLapkin what do you want to do here? Do you want to make a change to tidy?

@jackh726 jackh726 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 26, 2022
@WaffleLapkin
Copy link
Member Author

@jackh726 I'm not sure if it's worth it, given the comments above...

But if the tidy check makes sense then I would gladly implement it.

@GuillaumeGomez
Copy link
Member

I think writing a tidy check is definitely a good idea.

@wesleywiser
Copy link
Member

While I'm very sympathetic to efforts to improve documentation hygiene and style, I don't think we should merge changes to things like formatting that can't be automatically checked by tidy as that either shifts the burden of maintaining the correct style onto reviewers or causes an unending stream of "incorrect" PRs and subsequent "fix style of x" PRs.

Therefore, if we want to adopt this style, I feel like it should be tidy checkable.

@rustbot rustbot added the A-testsuite Area: The testsuite used to check the correctness of rustc label Jan 12, 2023
@rust-cloud-vms rust-cloud-vms bot force-pushed the no-double-spaces-in-comments branch 2 times, most recently from ba8e7f3 to 4156450 Compare January 12, 2023 20:02
@WaffleLapkin
Copy link
Member Author

I've added a tidy check and removed rustc_apfloat changes.

@bors
Copy link
Contributor

bors commented Jan 13, 2023

☔ The latest upstream changes (presumably #101138) made this pull request unmergeable. Please resolve the merge conflicts.

@rust-cloud-vms rust-cloud-vms bot force-pushed the no-double-spaces-in-comments branch from 4156450 to bd81463 Compare January 13, 2023 09:02
@WaffleLapkin
Copy link
Member Author

@Mark-Simulacrum can you review tidy changes?

@jackh726
Copy link
Member

Can you move the whitespace changes in the tidy commit to the first commit?

After that, r=me.

@rust-cloud-vms rust-cloud-vms bot force-pushed the no-double-spaces-in-comments branch from bd81463 to 7d59c0c Compare January 17, 2023 08:10
@WaffleLapkin
Copy link
Member Author

@bors r=jackh726

@bors
Copy link
Contributor

bors commented Jan 17, 2023

📌 Commit 7d59c0c has been approved by jackh726

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 17, 2023
@matthiaskrgr matthiaskrgr mentioned this pull request Jan 17, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 17, 2023
@bors
Auto merge of rust-lang#106998 - matthiaskrgr:rollup-hmfisji, r=matth…
edefa41 
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#104505 (Remove double spaces after dots in comments)
 - rust-lang#106784 (prevent E0512 from emitting [type error] by checking the references_error)
 - rust-lang#106834 (new trait solver: only consider goal changed if response is not identity)
 - rust-lang#106889 (Mention the lack of `windows_mut` in `windows`)
 - rust-lang#106963 (Use `scope_expr_id` from `ProbeCtxt`)
 - rust-lang#106970 (Switch to `EarlyBinder` for `item_bounds` query)
 - rust-lang#106980 (Hide `_use_mk_alias_ty_instead` in `<AliasTy as Debug>::fmt`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 68f1233 into rust-lang:master Jan 18, 2023
@rustbot rustbot added this to the 1.68.0 milestone Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eddyb eddyb eddyb requested changes

@GuillaumeGomez GuillaumeGomez GuillaumeGomez approved these changes

Assignees

@jackh726 jackh726

Labels
A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.68.0
Development

Successfully merging this pull request may close these issues.
10 participants
@WaffleLapkin @rustbot @GuillaumeGomez @saethlin @bors @rust-log-analyzer @jackh726 @oli-obk @wesleywiser @eddyb