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

Various cosmetic and minor changes #57016

Closed
wants to merge 1 commit into from

Conversation

alexreg
Copy link
Contributor

@alexreg alexreg commented Dec 20, 2018

r? @Centril :-D

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 20, 2018
@Centril
Copy link
Contributor

Centril commented Dec 20, 2018

For the purposes of review I'd prefer if you could:

  1. separate code changes from changes to comments into different PRs.
  2. separate various changes with done with regexes from each other into different PRs.

That's both easier to review and much easier to get through bors.

@rust-highfive

This comment has been minimized.

@alexreg
Copy link
Contributor Author

alexreg commented Dec 20, 2018

@Centril sure, will do later.

@scalexm
Copy link
Member

scalexm commented Dec 20, 2018

Before provoking a lot of merge conflicts again, would it be possible for some other people of e.g., @rust-lang/docs @rust-lang/compiler to confirm that these kinds of PRs are wanted on a regular basis?

@@ -669,7 +669,7 @@ impl char {
}
}

/// Returns true if this `char` is alphanumeric, and false otherwise.
/// Returns whether this `char` is alphanumeric, and false otherwise.
Copy link
Contributor

Choose a reason for hiding this comment

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

The "and false otherwise" part no longer makes sense if this construction is used. (Same applies to many other places.)

Copy link
Member

Choose a reason for hiding this comment

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

I prefer true if but not being a native english speaker, not sure which is "better"...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

“Whether” is definitely better, as a native speaker. They’re both grammatically correct, but “whether” is more succinct and to the point. It’s like writing boolval as opposed to if boolval { true } else { false }, which is an anti-pattern by anyone’s measure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nikic Good point, I'll remedy that. It's the problem with find-and-replace.

Copy link
Member

Choose a reason for hiding this comment

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

I personally think true if is a bit better/easier. I don't find it more succinct; it's literally the same number of characters. Additionally, true if is something that's very direct for programmers, even if whether might make more sense outside of a programming context.

Copy link
Contributor

Choose a reason for hiding this comment

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

Given my experience with English teachers (I was accepted to grad school in English), in my experience they're much less dogmatic than that.

What you call dogma I call respect for other people's time. Let's not fill this thread with anecdotes about our various formal-English-writing teachers, ye? I've had them too and they would care about this.

My view is that, we should aspire to the same standards academic papers have in at least the reference and imo also the standard library. The rustc code-base doesn't need the same level of rigor. We should however also not try to be intentionally casual when someone does the work, as @alexreg has, of improving writing style.

Copy link
Member

@steveklabnik steveklabnik Dec 30, 2018

Choose a reason for hiding this comment

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

I fundamentally do not believe this to be an improvement.

(The other changes? Very much so. This one? Not so much.)

Copy link
Contributor

Choose a reason for hiding this comment

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

@steveklabnik Reasonable minds can disagree. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

“Whether” is definitely better, as a native speaker

I probably prefer true if exactly because I'm not a native speaker.
I probably can't even pronounce "whether" properly, and it took me years to discern it from "weather".
It's better for technical documentation to use some kind of simplified, a bit pseudocode-like version of English, to be clear for larger audience. There's a long history behind this general idea.

Copy link
Contributor Author

@alexreg alexreg Jan 1, 2019

Choose a reason for hiding this comment

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

@petrochenkov If you had to pronounce the word when reading documentation, you'd possibly have a good point... (it's pronounced exactly the same in most English dialects, but is almost always easy to distinguish from context). In any case, "whether" is definitely part of elementary English vocabulary, so I'm not sure how strong an argument it is for written English.

@steveklabnik While do I strongly feel this is an improvement (it's not a matter of dogma so much as expressivity or directness, to me), I am however happy to let this stand as a subjective point, and defer to your preference, if it helps get this PR merged. :-) I know the PR will be discussed by several people in the next lang team meeting though, so it presumably won't be a decision taken by one person, at least not up-front...

@nikomatsakis
Copy link
Contributor

Regarding whether these sorts of changes are desirable, i'm of two minds. They are annoying when it comes to merge conflicts -- but on the other hand it is generally nice to have a codebase with typos fixed and things "cleaned up and consistent" (that said, I think some of these changes -- e.g., changing to "returns whether" from "returns true if" -- are not clearly better to me, just different).

I guess in general my attitude has been "hey, if somebody wants to make the changes and they seem neutral-to-good, why not".

Curious though to hear what others think.

One thing I definitely do prefer is to see the changes factored out into distinct commits (or, in this case, PRs).

@petrochenkov
Copy link
Contributor

-0.5 from me.
I'm ok with formatting changes like "two spaces" -> "one space" (they would better be done by rustfmt though) and with "variable_name" -> "variable_name".
But other changes either make things worse ("true if" -> "whether"), or longer ("vs" -> "versus", moving comments to separate lines), or weirder ("def-id" -> "def-ID"), or just randomly change things in accordance with some subjective rules that I can't infer.
Also, churn is bad in general, I want git blame to lead to people who actually wrote the code rather than formatting changes.

@alexreg
Copy link
Contributor Author

alexreg commented Dec 20, 2018

@scalexm You’re really trying to make life difficult for me, aren’t you haha? 😄 I’m only trying to improve the messy aspects of the codebase.... Anyway I’ll consult team members on this.

@scalexm
Copy link
Member

scalexm commented Dec 20, 2018

@alexreg No that isn’t my goal at all! It’s just that I am unsure this is wanted by everyone, so as part of this community, I’d like at least a few voices to support that kind of changes before moving forward. Of course if there is a general consensus I’ll have no further objections.

@alexreg
Copy link
Contributor Author

alexreg commented Dec 20, 2018

@scalexm Fair enough. I was only half serious anyway. It's certainly worth getting the opinion of the compiler team, whose opinions matter most here, followed by frequent contributors to the codebase like you or me. I know you're against it mainly, but let's keep weighing things up...

@alexreg
Copy link
Contributor Author

alexreg commented Dec 20, 2018

@petrochenkov

But other changes either make things worse ("true if" -> "whether"), or longer ("vs" -> "versus", moving comments to separate lines), or weirder ("def-id" -> "def-ID"), or just randomly change things in accordance with some subjective rules that I can't infer.

"Whether" is definitely better, for the reason I stated above. I think you'll either have to trust a native speaker on this (though I'm sure you could find some who dispute my view)... or we'll just agree to disagree. :-)

I don't think "ID" is weirder. "ID" is always capitalised in English, so why not here too, as part of a hyphenated word? The problem is, all rules of natural language are subjective to varying degrees. There's no black and white. My discussion started on the internals forum isn't going to make progress any time soon, if it does, so this is the next best thing in my view.

Also, churn is bad in general, I want git blame to lead to people who actually wrote the code rather than formatting changes.

I think the downside is greatly exaggerated. One can easily look beyond the last edit. The idea is that going forwards, people will hopefully start conforming to a style consensus, even before it gets formalised (if it ever does). I do realise it's marginally harder to look at the blame history of a line or file beyond a certain commit, but it's only marginally.

@nikomatsakis I basically agree with that. On the plus side, these sorts of large cosmetic PRs will be few and far-between, and the conflicts are a bit annoying, but not that bad (usually very easy and quick to fix in my experience). I guess it's half me feeling in some sort of "obsessive-compulsive" frame of mind that makes me do these things, but I also genuinely feel they contribute to the long-term health of the codebase. :-)

@bors
Copy link
Contributor

bors commented Dec 21, 2018

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

@rust-highfive

This comment has been minimized.

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:0161cece:start=1545370749328916942,finish=1545370752255872052,duration=2926955110
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0
---
[01:01:04] ..........................................ii........................................................ 3600/5192
[01:01:05] ............................................................i....................................... 3700/5192
[01:01:07] .................................................................................................... 3800/5192
[01:01:08] .................i.................................................................................. 3900/5192
[01:01:11] ........................F........................................................................... 4000/5192
[01:01:24] .................................................................................................... 4200/5192
[01:01:27] .................................................................................................... 4300/5192
[01:01:31] .......................................................i............................................ 4400/5192
[01:01:37] .................................................................................................... 4500/5192

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 @TimNN. (Feature Requests)

@Zoxc
Copy link
Contributor

Zoxc commented Dec 21, 2018

I generally want to avoid churn unless there is a clear non-subjective improvement.

For the changes in this PR, there are several things I disagree with:

  • The removal of github links. I really want to keep those as it's very clear what they refer to and they are easy to open with a click.
  • The use of backticks in comments. I think that just adds unnecessary noise.
  • Not capitalizing sentences in FIXME comments.

@bors
Copy link
Contributor

bors commented Dec 21, 2018

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

@alexreg
Copy link
Contributor Author

alexreg commented Dec 21, 2018

@Zoxc Let me offer some rationale.

  • The removal of github links. I really want to keep those as it's very clear what they refer to and they are easy to open with a click.

a) There was inconsistent use of "issue #xyz" vs. URLs, b) URLs are noisy in comments and make them harder to read

  • The use of backticks in comments. I think that just adds unnecessary noise.

It makes it very clear what is not part of the normal English sentence (not just code, but metavariables and other expressions), and what isn't. Seems you're in a minority here, but not sure.

  • Not capitalizing sentences in FIXME comments.

This is minor, of course. It's a standard grammatical rule not to follow a colon with capitalisation though.

@nikic
Copy link
Contributor

nikic commented Dec 21, 2018

This is minor, of course. It's a standard grammatical rule not to follow a colon with capitalisation though.

Style guides sometimes (e.g. AP) distinguish whether the words after the colon are a dependent clause, and use capitalization if it isn't. In the case of TODO/FIXME, what follows will usually not be a dependent clause and thus should be capitalized. Sometimes a distinction is made based on whether the colon is followed by one or by more than one sentences (e.g. Chicago).

I don't particularly care either way, but I think it's important to acknowledge that these changes are ultimately based on your personal preference in the choice of a particular style guide, not on normative rules of the English language. And that makes it hard to agree on things, because people will have different personal preferences.

@alexreg
Copy link
Contributor Author

alexreg commented Dec 21, 2018

I don't particularly care either way, but I think it's important to acknowledge that these changes are ultimately based on your personal preference in the choice of a particular style guide, not on normative rules of the English language. And that makes it hard to agree on things, because people will have different personal preferences.

This may be an American English thing. In fact, I just looked it up, and apparently we Brits always use lower case after colons, but Americans often follow the rule you stated above.

@Centril
Copy link
Contributor

Centril commented Dec 21, 2018

I mostly agree with @nikomatsakis.

On a tangent, I think that a more serious problem and a barrier to entry is the huge size of some files and functions/methods in the rust repo. Having >= 7000 LOC files strikes me as unmaintainable / unreadable and might point to larger architectural problem around APIs that are hard to refactor (possibly due to having too much context / being too stateful). A more healthy size would be ~300-1000 LOC per file imo.

@rust-highfive

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Dec 22, 2018

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

@rust-highfive

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Dec 26, 2018

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

@Centril Centril assigned nikomatsakis and unassigned Centril Dec 28, 2018
@nikomatsakis nikomatsakis added I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 28, 2018
@nikomatsakis
Copy link
Contributor

In general, I would be careful @alexreg in declaring anything to be "objectively" better, particularly when it comes to English, which is certainly an unruly language lacking any kind of "centralized" authority (I can't say if the same is true for other languages). =)

Definitely things like whether to capitalize after a colon have a variety of rules (amusingly, this exact question came up recently for me as I was trying to edit some of my daughter's homework, and couldn't predict what rules the teacher might be using).

My personal take is that I prefer backticks in comments to identify variable names and the like and don't really care about the rest. e.g., a URL is fine, but it's also ok to write #2123, as long as I can easily find the thing in question.

As for the fate of this PR, I'm not yet sure what to do exactly. As I wrote before, I don't object to merging it. I do however have reservations about trying to standardize rules on this sort of thing -- I'm not eager to be nitpicking PRs on random grammatical rules like whether to capitalize a sentence fragment after the word "whether". (Note: Here I am referring specifically to internal comments -- public-facing documentation is another thing.)

One thing I did notice is that this PR edits both internal rustc APIs as well as the documentation for things in libstd. It does strike me as reasonable to have a "style guide" for the latter, but I would expect @rust-lang/docs to be involved in developing it. It might be wise to separate out those changes, ultimately, not sure.

For the time being, I've nominated the PR for discussion at a @rust-lang/compiler meeting.

@steveklabnik
Copy link
Member

steveklabnik commented Dec 28, 2018 via email

@nikomatsakis
Copy link
Contributor

The docs team has two merged RFCs related to style for public comments, incidentally.

Yes, but I don't recall whether they addressed these specific questions.

@@ -1388,7 +1388,7 @@ impl String {
self.vec.len()
}

/// Returns `true` if this `String` has a length of zero.
/// Returns whether this `String` has a length of zero.
Copy link
Contributor

Choose a reason for hiding this comment

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

Example of a change affecting public-facing documentation. That feels to me like a @rust-lang/docs decision.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point. The quality of public-facing documentation in rustc currently isn't great (at least compared to the stdlib and some libraries), but it would be nice if it reaches that level some day, and if the style is consistent with the stdlib.

@alexreg
Copy link
Contributor Author

alexreg commented Dec 28, 2018

Thanks for the nomination @nikomatsakis. English does lack a central authority, as you say (Spanish and French notably have one, I believe). That said, there are certain things that are for all intents and purposes objective, and others that are more subjective or dialectal – they all fall on a spectrum though.

@rust-highfive

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Dec 29, 2018

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

@bors
Copy link
Contributor

bors commented Jan 1, 2019

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

@bors
Copy link
Contributor

bors commented Jan 1, 2019

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

@pnkfelix
Copy link
Member

Discussed at T-compiler meeting. Team essentially decided that we do not have any existing style manual dictating what kind rules we follow for our comments, and we are nervous about adopting any such style manual for e.g. what constitutes proper English.

There was general consensus that with the PR as given, its too difficult for us to immediately see distinct categories of the changes here, and therefore it would represent some non-trivial amount of work to build up such a categorization. Which means we cannot effectively have a conversation about the set of changes here, at least not in an exhaustive manner. (So not only can we not accept the PR as is, but we really can't provide great feedback on how to refine it, apart from: Can you break this up into separate commits or even PR's that are themselves separated by category. Which then would represent a lot of potential busy work for you with little payoff.)

We also recognized that discussions on formatting and comment styles are prone to bikeshed and conflict: a potential big drain on developer time.


Our basic consensus on how to move forward is this:

  • We generally accept drive-by style changes, in the sense that if you happen to be working on a purposeful change to code near by, then people will accept obvious improvements to comment text near there. (But if anyone objects to a change in style, then we will tend to error on the side of keeping the text as is.)
  • Overall, we just try not to make a big deal about any of this, and do not want to adopt any true "official" rules.

@pnkfelix
Copy link
Member

closing PR under reasoning given in my previous comment

@pnkfelix pnkfelix closed this Jan 17, 2019
@alexreg
Copy link
Contributor Author

alexreg commented Jan 17, 2019

@pnkfelix So to be clear, if I separate this into a bunch of commits, categorised, would you or another team member take another look at it? I think some of these changes dramatically improve the codebase readability still.

@estebank
Copy link
Contributor

@alexreg it'd be even better if they could be done as separate PRs to avoid one type of changes from blocking the others from being merged expeditiously.

@alexreg
Copy link
Contributor Author

alexreg commented Jan 17, 2019

@estebank Yeah, but there might be a lot of conflicts then, so I'm very much not a fan of it. It's also just a PITA for me.

@oli-obk
Copy link
Contributor

oli-obk commented Jan 18, 2019

Yeah, but there might be a lot of conflicts then, so I'm very much not a fan of it. It's also just a PITA for me.

I'm not sure if helpful (since you did not mention which parts you find painful), but the way I do PRs like that is by only having a single PR open. Once split into multiple commits, you can open a PR for the first commit, once that is merged, open a PR for the second commit, ... When rebasing, you rebase the PR-branch and then the branch with all commits on top of that. This way you only use up a single workspace and don't need to worry about cross-conflicts between separate PRs.

@alexreg
Copy link
Contributor Author

alexreg commented Jan 18, 2019

@oli-obk Sounds fair.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.