prefer if let to match with None => () arm in some places
#34268
Conversation
|
r? @pnkfelix (rust_highfive has picked a reviewer for you, use r? to override) |
|
I think we do care. Everything looks good to me. Do r+ if Travis passes. (Here is bors documentation.) @bors delegate+ |
|
✌️ @zackmdavis can now approve this pull request |
| span_line.line == comment_line.line { | ||
| self.print_comment(cmnt)?; | ||
| self.cur_cmnt_and_lit.cur_cmnt += 1; | ||
| } |
There was a problem hiding this comment.
The body of this if statement is indented too much -- it should be:
if span.hi < (*cmnt).pos && (*cmnt).pos < next &&
span_line.line == comment_line.line {
self.print_comment(cmnt)?;
self.cur_cmnt_and_lit.cur_cmnt += 1;
}There was a problem hiding this comment.
@jseyfried Sure, I like that; I was just accepting the indentation Emacs gave me. Is this "when breaking up an if-condition onto multiple lines, the condition continuation line gets a four-space indent, but the body of the conditional only gets one additional space's worth of indent beyond that" rule documented anywhere? (I don't see anything in src/doc/style/style/whitespace.md.) Is so, this would be a bug (or desirable feature-request) in Emacs rust-mode.
There was a problem hiding this comment.
@zackmdavis I believe the rule is "when breaking up an if-condition onto multiple lines, the condition continuation line gets a three-space indent (so that it lines up with the first line of the if-condition), and the body is indented four spaces as usual".
I'm not sure if this is documented anywhere, but I've seen it a lot in the Rust code base. I've also seen other styles for the if-condition, like
if span.hi < (*cmnt).pos && (*cmnt).pos < next
&& span_line.line == comment_line.line {but the body should always be indented four spaces no matter what.
There was a problem hiding this comment.
three-space indent [...] body is indented four spaces as usual
Oh, of course; it looks like I miscounted last night (I was sleepy).
zackmdavis
force-pushed
the
if_let_over_none_unit_arm
branch
from
fc34b21 to
332dc05
Compare
|
|
| } | ||
| if let Some(impl_list) = | ||
| self.tcx.inherent_impls.borrow().get(&self.tcx.map.local_def_id(id)) { | ||
| for impl_did in impl_list.iter() { |
There was a problem hiding this comment.
nit: this line and the following block are indented 5 spaces relative to the if let -- should be 4.
Agreed -- I almost always prefer a clean/meaningful history to a "historically accurate" history. |
zackmdavis
force-pushed
the
if_let_over_none_unit_arm
branch
from
332dc05 to
824d529
Compare
Casual grepping revealed some places in the codebase (some of which antedated `if let`'s December 2014 stabilization in c200ae5a) where we were using a match with a `None => ()` arm where (in the present author's opinion) an `if let` conditional would be more readable. (Other places where matching to the unit value did seem to better express the intent were left alone.) It's likely that we don't care about making such trivial, non-functional, sheerly æsthetic changes. But if we do, this is a patch.
zackmdavis
force-pushed
the
if_let_over_none_unit_arm
branch
from
824d529 to
8531d58
Compare
|
"Semantic indents are always four spaces; if a long if-condition must be broken onto multiple lines, the continuation line may receive either a three-space or eight-space indent, to taste"? |
|
Force-pushed again. My reading comprehension was not in best form last night, but I think the whitespace should be good now. |
|
Great! @bors r+ |
|
📌 Commit 8531d58 has been approved by |
…m, r=jseyfried prefer `if let` to match with `None => ()` arm in some places Casual grepping revealed some places in the codebase (some of which antedated `if let`'s December 2014 stabilization in c200ae5a) where we were using a match with a `None => ()` arm where (in the present author's opinion) an `if let` conditional would be more readable. (Other places where matching to the unit value did seem to better express the intent were left alone.) It's likely that we don't care about making such trivial, non-functional, sheerly æsthetic changes. But if we do, this is a patch.
This is a spiritual succesor to rust-lang#34268/8531d581, in which we replaced a number of matches of None to the unit value with `if let` conditionals where it was judged that this made for clearer/simpler code (as would be recommended by Manishearth/rust-clippy's `single_match` lint). The same rationale applies to matches of None to the empty block.
…=jseyfried
prefer `if let` to match with `None => {}` arm in some places
This is a spiritual succesor to #34268 / 8531d58, in which we replaced a
number of matches of None to the unit value with `if let` conditionals
where it was judged that this made for clearer/simpler code (as would be
recommended by Manishearth/rust-clippy's `single_match` lint). The same
rationale applies to matches of None to the empty block.
----
r? @jseyfried
In rust-lang#34268 (8531d58), we replaced matches of None to the unit value `()` with `if let`s in places where it was deemed that this made the code unambiguously clearer and more idiomatic. In rust-lang#34638 (d37edef), we did the same for matches of None to the empty block `{}`. A casual observer, upon seeing these commits fly by, might suppose that the matter was then settled, that no further pull requests on this utterly trivial point of style could or would be made. Unless ... It turns out that sometimes people write the empty block with a space in between the braces. Who knew?
…empty_block_arm, r=nikomatsakis
prefer `if let` to match with `None => { }` arm in some places
In rust-lang#34268 (8531d58), we replaced matches of None to the unit value `()`
with `if let`s in places where it was deemed that this made the code
unambiguously clearer and more idiomatic. In rust-lang#34638 (d37edef), we did
the same for matches of None to the empty block `{}`.
A casual observer, upon seeing these commits fly by, might suppose that
the matter was then settled, that no further pull requests on this
utterly trivial point of style could or would be made. Unless ...
It turns out that sometimes people write the empty block with a space in
between the braces. Who knew?
Casual grepping revealed some places in the codebase (some of which
antedated
if let's December 2014 stabilization in c200ae5a) where wewere using a match with a
None => ()arm where (in the presentauthor's opinion) an
if letconditional would be more readable. (Otherplaces where matching to the unit value did seem to better express the
intent were left alone.)
It's likely that we don't care about making such trivial,
non-functional, sheerly æsthetic changes.
But if we do, this is a patch.