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

Tracking issue for RFC 1925: Allow an optional vert at the beginning of a match branch #44101

Closed
1 of 3 tasks
aturon opened this issue Aug 26, 2017 · 16 comments
Closed
1 of 3 tasks
Labels
B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@aturon
Copy link
Member

aturon commented Aug 26, 2017

This is a tracking issue for the RFC "Allow an optional vert at the beginning of a match branch " (rust-lang/rfcs#1925).

Steps:

@aturon aturon added B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Aug 26, 2017
@KiChjang
Copy link
Member

I'm interested in looking into this... I suspect most of the code that I need to edit will be under libsyntax, correct?

@KiChjang
Copy link
Member

Looks like I was too slow, @mattico has went ahead and created a PR.

@mattico
Copy link
Contributor

mattico commented Aug 26, 2017

I should get in the habit of commenting before working on things... but most things I start I don't finish 😄.

@Mark-Simulacrum Mark-Simulacrum added the C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC label Aug 26, 2017
bors added a commit that referenced this issue Sep 2, 2017
@nikomatsakis nikomatsakis added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. E-needs-mentor labels Sep 15, 2017
@nikomatsakis nikomatsakis added this to the impl period milestone Sep 15, 2017
@nikomatsakis nikomatsakis removed this from the impl period milestone Sep 15, 2017
@nikomatsakis nikomatsakis removed the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 16, 2017
@nikomatsakis
Copy link
Contributor

@rfcbot fcp merge

I propose that we stabilize this (rather small) feature. The implementation has been done for some time now. At present, there is only one test:

I think I'd prefer some "positive" tests showing it doing the right thing if the feature gate is enabled, but in this case the feature is so simple that is probably not necessary.

@rfcbot
Copy link

rfcbot commented Oct 16, 2017

Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Oct 16, 2017
@cramertj
Copy link
Member

@rfcbot reviewed

@rfcbot
Copy link

rfcbot commented Jan 18, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Jan 18, 2018
@ghost
Copy link

ghost commented Jan 19, 2018

The comma is redundant, cause of there is a vertical bar preposed.

@rfcbot
Copy link

rfcbot commented Jan 28, 2018

The final comment period is now complete.

kennytm added a commit to kennytm/rust that referenced this issue 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
kennytm added a commit to kennytm/rust that referenced this issue 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 issue 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
@vorner
Copy link
Contributor

vorner commented Mar 26, 2018

Hello

I don't really want to hijack this issue, but I don't see which other channel to use and it is a bit related… Please point me to another place if there's a better one.

The way this got accepted made me a bit sad. From discussions on Gitter, it seems I'm not the only one. I'm not really writing to complain, or to sound disrespectful, but to point out to a possible problem with the RFC process this might point to. It's also possible it's just my impression and if it's not really a problem, I apologize.

The original RFC was accepted despite many people giving it thumps down, but not complaining very loudly. The point then was „let's try it if it works“ rust-lang/rfcs#1925 (comment). What I think happened then was that nobody really tried it out during the baking time (because not many people liked it and there was no motivation to try it out, not like eg. conservative_impl_trait ‒ you don't really make your code compile on nightly only unless there's a real benefit to it). Then this got stabilized quite silently, without any further discussion. I think the feeling from the many thumps-down could be that it's a bit controversial and many people now will feel like this got passed somehow secretly.

I know all these things are made public, so if someone really did want to protest, they could. And I don't want to sound like I think there was an ill intention from anyone. But it's too much to keep track with a day job that isn't especially about rust and given the mostly quiet but numerous disagreement of the original proposal might have warranted some kind of alert to the community, if someone indeed did try it out during the time. Also, is there some way to give feedback on these yet unstable features? They are there to be tested, but unless it is something high-profile, the impression (and drive to try them out) is pretty non-existent. In general, I have a feeling of these things being a bit rushed, at least compared to previous times.

Again, I apologize if it's just me, but I wanted to point the problem out before it leads to some more serious problems than just some people being sad (after all, this change to the language is rather small).

@aturon
Copy link
Member Author

aturon commented Mar 26, 2018

@vorner, thanks much for your carefully-written and very thoughtful comment. I completely understand your concerns.

As a general rule RFCs/feature decisions are based on consensus within the relevant team, not necessarily the entire community. The teams are required to take arguments against an RFC very seriously, and address them on thread, but they may still decide the concerns don't outweigh the benefits. All that said, though, in general we strive for alignment with the broader community, and very rarely accept RFCs where there is significant negative feedback, even if the team disagrees; it's taken as a sign that more discussion is needed.

FWIW, I think in this case a significant part of what happened comes down to the feature being so insignificant/low commitment. Thus, even though there was an unusual amount of negative sentiment on thread, the Lang Team still felt comfortable making a call based on their own internal consensus.

However, I think it would've been good to note that lack of broad consensus in the tracking issue, to ensure that we fully revisited the concerns.

Also, is there some way to give feedback on these yet unstable features?

Tracking issues, like this one, are the established place to leave feedback on unstable features. Stabilization always requires reviewing these comments threads and ensuring that any feedback is handles. Similarly, even when the team believes a feature is ready for stabilization, it goes into a public "final comment period" (with corresponding tag), and is advertised in This Week In Rust. This is all done to try to strike a balance between making progress and ensuring that everyone has an ample opportunity to participate.

@pravic
Copy link
Contributor

pravic commented Mar 27, 2018

Stabilization always requires reviewing these comments threads and ensuring that any feedback is handles.

Was it true in this case? Because it looks like the stabilization was accepted immediately: #47947 (comment)

@petrochenkov
Copy link
Contributor

As a general rule RFCs/feature decisions are based on consensus within the relevant team, not necessarily the entire community.

I think we need some kind of veto on accepting "obviously disliked" RFCs even if lang team's sentiment differs from average.
It shouldn't affect the process much, I remember maybe a couple of cases similar to this at most.

@pravic

it looks like the stabilization was accepted immediately: #47947 (comment)

I r+'d because previously the whole lang team approved stabilization (#44101 (comment)).
(I don't personally like this feature either.)

@vorner
Copy link
Contributor

vorner commented Mar 27, 2018

I think we need some kind of veto on accepting "obviously disliked" RFCs even if lang team's sentiment differs from average.

My point wasn't exactly about this. After all, someone has to do the decision eventually and letting the people who work full-time on that sounds reasonable.

However, I tried to point out two things:

  • Considering the way the RFC was accepted, I'd expect some kind of discussion or at least a note explaining why the discussion didn't really happen. This way it looks more like nobody really cared about a small thing like this, so it got accepted and swept out of the way, forgetting about the dislike of the original proposal.
  • The current process leans to accept a feature nobody cares much either way. I think it would be more healthy for the language to default to dropping it, as not to clutter the language too much.

@Ixrec
Copy link
Contributor

Ixrec commented Mar 27, 2018

The current process leans to accept a feature nobody cares much either way. I think it would be more healthy for the language to default to dropping it, as not to clutter the language too much.

As a spectator, I strongly disagree that there's any such trend. I see plenty of RFCs where nobody feels particularly strongly--or where the strong feelings are not converging on a consensus--get closed or postponed all the time. In fact, this is the only RFC I can think of where anyone even suggested that the lang team accepted a feature without due process.

Considering the way the RFC was accepted, I'd expect some kind of discussion or at least a note explaining why the discussion didn't really happen. This way it looks more like nobody really cared about a small thing like this, so it got accepted and swept out of the way, forgetting about the dislike of the original proposal.

To me, "some kind of discussion" already happened on the RFC thread. In particular, rust-lang/rfcs#1925 (comment) seems like a great answer to the "how did this get accepted?" question:

Despite the thumb count, the comments felt much more positive. What seemed clear is that while many users didn't want this syntax (and maybe wouldn't allow it in their code base), there were some users who very strongly did want this and were willing to press on it. I weighed these strong positive feelings more heavily than the mild negative feelings.

And I didn't see any new arguments or implementation experience against this come to light during the time this was an unstable feature.


For completeness, I do think it would have been nice to get one case of somebody applying this feature to their codebase and saying "yep, I like this" before stabilizing it. For a feature this trivial and "low commitment", I'd be 100% on board with stabilization after seeing only one such comment. But I think the complete lack of any such comments (positive or negative!) on this issue is a major contributor to the resurfacing of these strong meta-feelings about the (superficial lack of?) stabilization process here, despite them having been (in my opinion) already properly addressed on the RFC thread.

So on that note, did any fans of the original RFC get a chance to try this out?

@warlord500
Copy link

warlord500 commented Mar 29, 2018

@Ixrec, I agree with everything you said! however in regards to the rust-lang/rfcs#1925 (comment), If a knew about this feature earlier, I would have made strong argument about why I disliked this feature! In fact, my feelings, I think can really be summed up to as hating this particular feature. simultaneously, their are very few things, I dislike about rust. if this feature, does happen then I plan to start an RFC to reverse it's decision!

while, I don't completely understand the RFC process. If this does get stabilized, I certainly don't see this
as failure of the RFC. rather of failure of those that might be strongly against It, not taking arms against it
early enough!

@vorner

After all, someone has to do the decision eventually and letting the people who work full-time on that sounds reasonable.

obviously, there is no thing that will be perfect to everyone. some one is going to have to take the blame for someone's idea of bad decision! My hope is that this feature doesn't become one of those!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests