-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Various minor/cosmetic improvements to code #56578
Conversation
Note: the import reordering/grouping is probably unnecessary in hindsight, since I hear people are working on running rustfmt on the source as a matter of process... that said, they don't hurt, and there are plenty of other good changes in there. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
cc me with nit fixed and when CI passes. |
This comment has been minimized.
This comment has been minimized.
☔ The latest upstream changes (presumably #54271) made this pull request unmergeable. Please resolve the merge conflicts. |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
This comment has been minimized.
This comment has been minimized.
@TimNN Why does @rust-highfive no longer hide CI log comments for out-of-date commits? |
☔ The latest upstream changes (presumably #56502) made this pull request unmergeable. Please resolve the merge conflicts. |
@bors r+ p=1 Let's get this done quickly so we avoid rebasing over and over again. |
📌 Commit 99fd42ff5dbd983dda8879578fe442ddeb9bac86 has been approved by |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@bors delegate+ |
✌️ @alexreg can now approve this pull request |
Aghh, nll stderr files... @bors r+ p=1 |
📌 Commit 003c5b7 has been approved by |
Various minor/cosmetic improvements to code r? @Centril 😄
☀️ Test successful - status-appveyor, status-travis |
I'm not sure I understand the added value of this PR, apart from forcing everyone to rebase, and I could not find any discussion about it prior to it being merged. I agree one should always feel free to fix typos or spelling / grammatical errors, but I'm not sure we should somehow "impose" a writing style or whatever. For example, I like to write the comma before the Latin locution "i.e." because I believe this is correct English and moreover it mimics the usage in my own language, but I don't care if someone else likes another usage. The same goes for abbreviating the Latin locution "nota bene". Everyone has their writing style :) I think this PR will be stale in a few weeks because people will probably stick to their own writing style. So what will be the next steps? Are we opening a PR once a month to fix people comments? Or will we impose PR authors to fix their comments before any PR can be merged? |
@scalexm I personally would like to see a writing style agreed upon. I adopted a comma after Latin abbreviations because it is the U.S. English convention, and such conventions are used everywhere else. (Note, I'm actually British.) I also consider it more logical, because "e.g." and "i.e." are essentially abbreviated Latin translations of the phrases "for example" and "that is". It is necessary in grammatically-correct English to include the comma:
You make a fair point about the codebase diverging in style, however, which is why I feel a consensus on writing style, punctuation, and whatnot would be great at this point. Reviewers could then uphold it. P.S. There's no requirement for any formal process on a PR like this, so I'm pretty sure no rules were broken. I dispute that it doesn't add value, though like I said above, it adds more value if people then stick to certain writing guidelines, and they're furthermore enforced. |
@alexreg That’s very fine but since it is a personal opinion, I’d like to see it discussed somewhere before imposing anything to the codebase, on the internals for example. |
I don't think there's any requirement to discuss it, as long as someone approves the PR... deciding on style guidelines is another matter. Do you want to create a thread for that, or should I? |
r? @Centril 😄