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

Stabilize feature(match_beginning_vert) #47947

Merged

Conversation

goodmanjonathan
Copy link
Contributor

@goodmanjonathan goodmanjonathan commented Feb 1, 2018

With this feature stabilized, match expressions can optionally have a | at the beginning of each arm.

Reference PR: rust-lang/reference#231

Closes #44101

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @petrochenkov (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Feb 1, 2018

📌 Commit a99b5db has been approved by petrochenkov

@petrochenkov petrochenkov added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Feb 1, 2018
@kennytm kennytm added the relnotes Marks issues that should be documented in the release notes of the next release. label Feb 2, 2018
@Mark-Simulacrum
Copy link
Member

@bors rollup

kennytm added a commit to kennytm/rust that referenced this pull request Feb 3, 2018
…inning_vert, r=petrochenkov

Stabilize feature(match_beginning_vert)

With this feature stabilized, match expressions can optionally have a `|` at the beginning of each arm.

Reference PR: rust-lang/reference#231

Closes rust-lang#44101
bors added a commit that referenced this pull request Feb 3, 2018
Rollup of 9 pull requests

- Successful merges: #47753, #47862, #47877, #47896, #47912, #47944, #47947, #47978, #47958
- Failed merges:
kennytm added a commit to kennytm/rust that referenced this pull request Feb 4, 2018
…inning_vert, r=petrochenkov

Stabilize feature(match_beginning_vert)

With this feature stabilized, match expressions can optionally have a `|` at the beginning of each arm.

Reference PR: rust-lang/reference#231

Closes rust-lang#44101
kennytm added a commit to kennytm/rust that referenced this pull request Feb 4, 2018
…inning_vert, r=petrochenkov

Stabilize feature(match_beginning_vert)

With this feature stabilized, match expressions can optionally have a `|` at the beginning of each arm.

Reference PR: rust-lang/reference#231

Closes rust-lang#44101
bors added a commit that referenced this pull request Feb 4, 2018
Rollup of 10 pull requests

- Successful merges: #47862, #47877, #47896, #47912, #47947, #47958, #47978, #47996, #47999, #47892
- Failed merges:
bors added a commit that referenced this pull request Feb 4, 2018
Rollup of 10 pull requests

- Successful merges: #47862, #47877, #47896, #47912, #47947, #47958, #47978, #47996, #47999, #47892
- Failed merges:
@bors bors merged commit a99b5db into rust-lang:master Feb 5, 2018
@goodmanjonathan goodmanjonathan deleted the stabilize_match_beginning_vert branch February 5, 2018 02:18
@stepancheg
Copy link
Contributor

RFC had 37 downvotes compared to 19 upvotes. Not obviously a good thing to be added to the language.

@warlord500
Copy link

I am very much against this being added to the language. it will lead to more confusing code!
looking at some of the code in Releases.md, i got mixed just looking at it

Undin added a commit to intellij-rust/intellij-rust that referenced this pull request Mar 26, 2018
Since rust 1.25.0 you can add '|' at the beginning of match arm.
See rust-lang/rust#47947.
These changes support new syntax
Undin added a commit to intellij-rust/intellij-rust that referenced this pull request Mar 26, 2018
Since rust 1.25.0 you can add '|' at the beginning of match arm (rust-lang/rust#47947).
These changes support this new syntax
@pravic
Copy link
Contributor

pravic commented Mar 27, 2018

Same here. But nobody cares despite

The teams are required to take arguments against an RFC very seriously

@warlord500
Copy link

this will be the one feature I will hugely disagree with in rust! unless their is extremely (strong willed) segment of developers that do like this. every example, I have been able to look at Involving, I find more confusing than with out the leading vertical bar! Not only that, but I have to yet to find strong evidence of many people wanting this feature! yes, there could be an example, but to the best of my knowledge, (which to be honest Isnt a lot) most code with this syntax added leads to more confusing code!

@TomasHubelbauer
Copy link
Contributor

For the record, while I don't have any idea how many people support this syntax, I do. I think F# uses vertical bars for match, too, and I personally am used to aligning union types this way in TypeScript, so it's welcome to have something familiar like this. Sure, the | symbol is also used for closures, but I believe the contexts are distinct enough for this not to matter.

@stepancheg
Copy link
Contributor

The problem is not with the leading bar itself, the problem is that Rust now has two syntaxes to express the same thing. F# supports only leading bars. If Rust deprecated syntax without leading bars, that would probably be OK.

@timbess
Copy link

timbess commented Mar 29, 2018

Yeah all the arguments behind this RFC make no sense to me. There were alternatives that worked already and looked just as good. "I got used to it in F# and now I want it in Rust" is a bit of a strange argument to me. Rust shouldn't be some Frankenstein's monster of parts of different languages. Ideas should be evaluated based on their merit, and the value this brings to the table is very questionable to me.

As for all the people arguing for flexible syntax and allowing different styles... This is how languages like Perl happened. "There is more than one way to do it" isn't a good thing; it's confusing and fragments the language. To quote Tim Peters "There should be one-- and preferably only one --obvious way to do it."

Maybe this particular change is alright, but I just think a lot of the arguments made in this RFC are very dangerous and might lead Rust down the path many other failed languages have taken. Let's learn from their mistakes rather than repeat them because I really want Rust to succeed.

There should be very strong arguments before we bloat the language spec more with little changes that have very minimal benefit to users and just add more mental overhead for beginners.

@Gedweb

This comment has been minimized.

bors bot added a commit to intellij-rust/intellij-rust that referenced this pull request Mar 31, 2018
2404: GRAM: support new syntax of match arms r=matklad a=Undin

Since rust 1.25.0 you can add `|` at the beginning of match arm (rust-lang/rust#47947)
These changes support this new syntax
Undin added a commit to intellij-rust/intellij-rust that referenced this pull request Mar 31, 2018
Since rust 1.25.0 you can add '|' at the beginning of match arm (rust-lang/rust#47947).
These changes support this new syntax

(cherry picked from commit 079a37a)
@valeth
Copy link

valeth commented Apr 6, 2018

This looks pretty useless to me, because you can't match (for me) obvious patterns like this:

let something = Some(1);
    
match something {
    | None
    | Some(x) => println!("something = {}", x)
};

Rust complains that the None pattern doesn't bind x.
It needs to be changed to this, which complete defeats the purpose of having the | in the first place.

let something = Some(1);
    
match something {
    | None    => (),
    | Some(x) => println!("something = {}", x)
};

Maybe I'm misunderstanding the feature, and there is a way to match such a pattern?

@H2CO3

This comment has been minimized.

@liigo

This comment has been minimized.

@valeth
Copy link

valeth commented Apr 20, 2018

Not really against having something similar to Haskell's guard statements.
But they need to at least have some practical use instead of just adding an optional syntax element that could easily be omitted.

@withoutboats
Copy link
Contributor

@valeth no one ever seems to have answered your question, but your comment seems a bit confused about the scope of this stabilization. Rust has had the ability to match multiple patterns with a single arm since before 1.0. there's no way to do what you suggest in your post, because x would be uninitialized in the None case.

This change was strictly syntactical: it makes |pat|pat equivalent to pat|pat in the same way that a,b, is equivalent to a,b.

@Arcaelyx
Copy link

This seems like such a useless pattern to add if it provides no extra functionality. In the end, things like this lead to people wanting their own style thrown in to vastly complicate the language without providing any extra practical benefit. Such as:

If we are to throw this in, then why don't we have ternary operators as well?
At least that actually leads to more concise code as opposed to now having
useless bloat with no additional benefit.

Whereas the difference with this is that it at least provides some benefit, but we start spiraling out of control with adding in features like this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.