-
Notifications
You must be signed in to change notification settings - Fork 2k
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
method k:v if condition #1871
Comments
Yes, I think it is. This is an example of the rewriter going too far, and wrapping the object around the postfix if statement ... when the postfix if statement should really be closing the implicit object. |
I bumped into this recently myself: I thought I'd be able to create an array of objects with
I feel like there's already an open issue on this... maybe @michaelficarra knows? |
+1, I've got this bug today. |
Yeah, I could swear this used to work properly. Perhaps a side-effect of satyr's big rewriter fixup? |
Nope, definitely not a recent change. I do believe there's an open issue for this. I'll try to find it. |
Implicit object is a diff{erent,icult} beast than implicit call in that it naturally spans multiple lines:
What should it mean?
|
As always, @satyr makes an excellent point. Naively, I would want:
... but that's fairly horribly inconsistent. Perhaps our current behavior is the best option? |
I don't think so.
Postfix |
Agreed with Michael. On Nov 18, 2011, at 10:47 AM, Michael [email protected] wrote:
|
Replace |
@satyr: yes, it's a little more complex than my over-simplification. In that case, the current compilation is what I would desire. |
I'm going to try to cut the Gordion knot that is @satyr's example: It seems we're all agreed that
well, the intuition is very different. So our options are:
I'm all for
|
+1 for |
Agreed: 👍 for 3. If you want the old behavior, wrap the postfixed expression in parentheses.
|
👍 for |
The object is |
@gavin Right, but then what about the case
? On Nov 20, 2011, at 9:51 PM, Gavin [email protected] wrote:
|
I guess |
Huh. @gravof's suggestion is actually something I've often wished for when conditionally passing (as opposed to not passing) a particular keyword argument to a function. +0.7, but let's save it until after the issue at hand is resolved? |
Apparently I'm in a special cases mood today, because I've opted for door number 2 -- special casing it so we get the intuitive behavior in both cases. The rule is this: IMPLICIT_ENDs (postfix So:
Means:
|
Shouldn't this compile too?
|
I think we should reconsider this inconsistency. If I have a program b: c if d and I use it as an operand of a lower-precedence operator, a = b: c if d The problem is that the first example is the only place where postfix conditions/loops have such a high precedence. The claimed use case fn
a: b
c: d if e
f: g is not convincing, and I don't think the inconsistency is worth it here. The user could easily parenthesise the value if that's what they intended. This would protect them from later edits changing their program in unexpected ways. |
While I agree I havn't used the |
Yes, it would have to be considered a breaking change, which we'd only make in a major version bump. I don't think this pattern is very common, and I'd be okay making it in CSR if it was approved by the community here first. CSR currently tries but fails to completely support this inconsistency. It would be much simpler, actually, to support the consistent precedence. |
I'm not perfectly clear on exactly what change you're suggesting here. Mind spelling it out a little bit more? Just removing the patch that closed this ticket? |
Yep, pretty much. |
If anyone wants to take a crack at it, I think it would be illuminating to see what changes in the test cases in the PR, with the optimization removed. |
+1 to fix this. I think that the patch hasn't considered the case of: f = ->
a: b if condition
# not consistent with
f = ->
a if condition and f = ->
a: b for i in c
#not consistent with
f = ->
a for i in c As far as I can tell the problem only arises in implicit returns, so we could, if we further dwelt in special-casing mood, fix that case and keep the current behavior. See other uses are working fine: x = a: b if c
x = (a: b for i in c)
method a:b if c Or we can just go with what Trevor proposed and others +1'd.
Can you show an example @michaelficarra ? Because I don't see an easy edit that would go from one compilation form to the other. |
from a = b: 1 if c
# to
a =
a: 2
b: 1 if c I guess |
Exactly, @Nami-Doc. |
Yeah, so with my suggestion this inconsistency stays: a = b: 1 if c
# vs
a =
b: 1 if c But I am not entirely sold on this being such a big problem, because I would expect each to compile as they do. (Then again, I was never a fan of postfix conditionals in the first place.) |
Fixes #1871, close implicit objects in implicit returns
method obj if condition
is compiled intoHowever,
method k:v if condition
is compiled intoI guess
method(k:v) if condition
is far more frequently intended thanmethod k: (v if condition)
. Is it reasonable that we change this precedence?The text was updated successfully, but these errors were encountered: