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

Drop 'when _' support #6150

Conversation

makenowjust
Copy link
Contributor

Ref #4837

I think this syntax has no use-case. It can be replaced with else block always.

Ref crystal-lang#4837

I think this syntax has no use-case.
It can be replaced with 'else' block always.
@makenowjust makenowjust force-pushed the fix/crystal-syntax/drop-when-underscore-support branch from a5c9de7 to 6fdaf42 Compare June 2, 2018 05:16
@bew
Copy link
Contributor

bew commented Jun 2, 2018

Note: this is currently invalid syntax:

case foo
else
  # bar
end

But this was valid:

case foo
when _
  # bar
end

I don't know the usecase of this, but it was spec-ed (in normalize/case_spec.cr), so there's probably a reason. Maybe macro-generated case node that could have no when or sth like that.

@makenowjust
Copy link
Contributor Author

IMHO case having only else block should be valid if it cause a problem in reality.

@ysbaddaden
Copy link
Contributor

I think it was added for not generating invalid code from a macro generating case statements that would happen to only generate the else part. You use when _ instead.

Probably we could support having only an else. I don't know why we chose this syntax.

@asterite
Copy link
Member

asterite commented Jun 2, 2018

To be honest, I didn't know that existed. I'm pretty sure I added it at one point because it's fancy, but not very useful.

Generating code in macros might be a reason to justify this, but we could just allow a single else instead, if we ever need this.

@asterite
Copy link
Member

asterite commented Jun 2, 2018

Or maybe it was added when we added tuple support in case... Which is another thing I think is a bit useless, and can be replaced with an if else chain (the need for tuple case is not super common, so using if else now and then is fine). I think we should remove that feature too.

@RX14 RX14 added this to the 0.25.0 milestone Jun 2, 2018
@RX14 RX14 merged commit 8da3b2e into crystal-lang:master Jun 2, 2018
@makenowjust makenowjust deleted the fix/crystal-syntax/drop-when-underscore-support branch June 2, 2018 15:38
@bcardiff
Copy link
Member

bcardiff commented Jun 2, 2018

It was added together with the tuple support and it make sense since _ can be used in other places:

_, b = expr

case tuple
when {0, _}
# ...
when {_, 0}
# ...
when {x, y}
# ...
end

I think we should have some minimum amount of time to merge PR so others has time to jump in ;-) 12hs in weekend for a syntax change seems to little time.

It is a corner case, I'm ok to the merge though.

@Sija
Copy link
Contributor

Sija commented Jun 2, 2018

It's definitely useful for pattern matching.

@asterite
Copy link
Member

asterite commented Jun 2, 2018

There's no such thing as pattern matching in Crystal. That's why _ was a bad idea, and why tuple in when was a bad idea. It makes it look like there's pattern matching, but there's not. So we should remove both.

Case is just an if with === or is_a?, but that's not pattern matching.

@makenowjust
Copy link
Contributor Author

A bad point of when _ I think is it can be placed anywhere. For example,

foo = 1

case foo
when _
  puts "when _"
when 1
  puts "when 1"
end

Which output is correct, when _ or when 1? Currently it shows when _. However someone expects when 1 is correct.

Such a syntax introducing ambiguity makes us confusing and it will be called as bad parts in future.

@straight-shoota
Copy link
Member

I think we should have some minimum amount of time to merge PR so others has time to jump in ;-)

yes, please!

chris-huxtable pushed a commit to chris-huxtable/crystal that referenced this pull request Jun 6, 2018
Ref crystal-lang#4837

I think this syntax has no use-case.
It can be replaced with 'else' block always.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants