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

Is an initial | on match arms a formatting issue? #119

Closed
nrc opened this issue Feb 18, 2018 · 8 comments
Closed

Is an initial | on match arms a formatting issue? #119

nrc opened this issue Feb 18, 2018 · 8 comments

Comments

@nrc
Copy link
Member

nrc commented Feb 18, 2018

It is now possible in Rust to start a match arm with a |, e.g.,

match foo {
    | whatever => {}
}

Currently Rustfmt treats a leading | or not as significant code, i.e., preserves what the user chose to do. However, it is not semantically significant, so one might consider it a style issue, in which case Rustfmt should change it one way or the other (since we believe in consistent formatting, not preserving user input). As a counter-point, we do not strip unnecessary () although these also are not semantically significant.

So, there are three options:

  • never use an initial |
  • always use an intial |
  • preserve the user choice.
@strega-nil
Copy link

strega-nil commented Feb 18, 2018

I personally want something like this: rust-lang/rustfmt#2234 . I think it would help a lot with the issue of indenting way too much in Rust.

@chriskrycho
Copy link

chriskrycho commented Feb 18, 2018

👍 to @ubsan's suggestion. This is where Reason landed, for example. Also, I've gotten quite used to the visual nicety of having every pattern in a union type prefaced by a | when declaring them in e.g. TypeScript, F♯, etc. It makes for a really pleasantly self-consistent style.

@ghost
Copy link

ghost commented Feb 18, 2018

It's worth noting that there's a subtle difference: | in Reason is actually equivalent to , in Rust. That's why in Reason we only have | at the beginning of line, while in @ubsan's suggestion we have both leading | and trailing ,.

While I can appreciate the reduction of rightward drift, my main worry is that having leading |s would further the perception of Rust as a sigil-heavy language. To be fair, leading | does look nice when matching on a large set of cases where some of them are indeed unified using |, but if there is no legitimate use of |, then perhaps writing it redundantly before each case is more harm than good? After all, legitimate uses of | are not very common.

And the problem with the second style (the one without indentation and without leading |) is that two adjacent closing braces can line up at the same indentation level:

match self.front() {
None => return None,
Some(e) => {
    if e.remove() {
        return Some(e);
    }
}
}

@steveklabnik
Copy link
Member

I really don't want leading |, so my preference is 1, then 3.

@jplatte
Copy link

jplatte commented Feb 19, 2018

As a counter-point, we do not strip unnecessary () although these also are not semantically significant.

Unnecessary () are something you use in very few situations, if ever, though. Initial | on match arms seem to me like something you would either add everywhere, or nowhere. So I would consider it a style choice.

@nrc
Copy link
Member Author

nrc commented Apr 12, 2018

OK, it sounds like there is consensus that this is a formatting issue, but not on exactly what we should do.

IMO, using | instead of an indent is too radical a change at this stage, and is also inconsistent with the rest of our formatting choices because we heavily rely on indentation rather than punctuation to identify 'sub-items'.

Therefore I propose we accept option 1, that tools should remove leading |s. However, tools could support an option like rust-lang/rustfmt#2234 (i.e., leading | without indentation).

nrc added a commit that referenced this issue Apr 12, 2018
Mike-Baker added a commit to Mike-Baker/rustfmt that referenced this issue Apr 14, 2018
This adresses issue rust-lang#2621

This commit turns out to be a partial revert of
ea3c01e

The rationale is that a `|` character preceding a match pattern is not
semantically relevant and therefore should be considered a
style/formatting choice.

A discussion concluded that the best way to emit consistant formatting
here was to strip the leading `|`

Discussion at rust-lang/style-team#119
Mike-Baker added a commit to Mike-Baker/rustfmt that referenced this issue Apr 14, 2018
This addresses issue rust-lang#2621

This commit turns out to be a partial revert of
ea3c01e

The rationale is that a `|` character preceding a match pattern is not
semantically relevant and therefore should be considered a
style/formatting choice.

A discussion concluded that the best way to emit consistent formatting
here was to strip the leading `|`

Discussion at rust-lang/style-team#119
@ghost
Copy link

ghost commented Apr 16, 2018

Rustfmt currently uses this formatting:

match foo {
    Foo::Bar(value)
    | Foo::Baz(_ignore_this, value)
    | Foo::Quux(_ignore_this, _and_this, value) => {
        println!("value = {}", value);
        Whatever::One
    }
    Foo::Spam(value) | Foo::Eggs(_, value) => Whatever::Two(value),
}

But the style with leading vert @withoutboats suggested here is starting to grow on me, and looks really neat in this case:

match foo {
    | Foo::Bar(value)
    | Foo::Baz(_ignore_this, value)
    | Foo::Quux(_ignore_this, _and_this, value)
    => {
        println!("value = {}", value);
        Whatever::One
    }
    | Foo::Spam(value)
    | Foo::Eggs(_, value)
    => Whatever::Two(value),
}

@nrc
Copy link
Member Author

nrc commented Apr 26, 2018

Closed by #127

@nrc nrc closed this as completed Apr 26, 2018
Mike-Baker added a commit to Mike-Baker/rustfmt that referenced this issue Jun 24, 2018
This addresses issue rust-lang#2621

This commit turns out to be a partial revert of
ea3c01e

The rationale is that a `|` character preceding a match pattern is not
semantically relevant and therefore should be considered a
style/formatting choice.

A discussion concluded that the best way to emit consistent formatting
here was to strip the leading `|`

This removes the match_with_beginning_vert test because it was asserting
the old behaviour which has been changed, it adds a new test
(issue_2621) which should be a more comprehensive check of the behavior
of `|` in match arms.

Discussion at rust-lang/style-team#119
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

No branches or pull requests

5 participants