-
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
Fix #4150: Correctly outdent ternary followed by method call #4535
Conversation
test/formatting.coffee
Outdated
.h() | ||
""" | ||
|
||
# Wrap calls containing spilling ternary |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please put a new line like test "wrap calls containing spilling ternary", ->
to start a new test.
Currently if anything inside this test
function fails, the whole test fails and it’s hard to know what part of this failed, since you’re testing multiple things here (the throws
and the two eq
s). It would be nice if each of those were separate test
functions, so that if one of them fails we know exactly which one.
While I agree that your example above should throw an error, this shouldn’t: gulp.pipe if 0 then 1 else g
a: 1
.h() I haven’t tested this, so I don’t know if your version does or doesn’t allow this. We should have a test though that makes sure the above still works as expected (i.e. |
id = (x) -> x | ||
result = f if true then 42 else id | ||
a: 2 | ||
.h |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@GeoffreyBooth I think you missed this test, it's exactly what you asked for.
I will split them each into a separate test call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s it different between the gulp.pipe
and result = f
lines? I agree they’re very similar, and should both work, but I was wondering if the assignment made a difference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, there is no difference. I can add a separate test if you want, but I think as long as this works it should be ok (it's hard to make this pass but your case fail).
3642c33
to
bd8b841
Compare
@GeoffreyBooth I split up the tests. |
Thanks. LGTM. @lydell? |
LGTM |
I think the code in #4150 is pretty horrible, and the fact that an inline ternary can continue after a method call in else just seems bizarre. But I do think that:
should not compile (certainly not as
.h()
being called on the inline object inside the ternary).So this PR makes it cause a syntax error, as it should. I would guess that implementation-wise this is not the right fix, but it works and I'm not an expert on the
rewriter
logic.