Conversation
…th literal expressions
froydnj
left a comment
There was a problem hiding this comment.
Don't mind me, I'm just completely misdiagnosing the issues here. 🤦♂️
Thanks for this fix!
|
🤦♂️ Of course, testing this out, there are now (a lot more?) cases where we aggressively multiline, so I think this netted out to more changes overall. |
|
Yeah I mean that's not wildly surprising -- I think checking for comments anywhere in the chain is probably ~not quite right, especially since in the Ripper implementation this was looking for tokens, and it didn't recurse so it probably isn't including comments inside breakables (like comments on method params in the chain, etc.). I'm not 100% sure how we could reliably reproduce that with Prism, the token check is pretty fundamentally different. Depending on how many there are and how feasible/annoying it is to land changes and so on, personally I think the Prism implementation before this change was the correct one, and I'd be fine to just revert it with the understanding that it would lead to changes. |
|
@froydnj Were you thinking we revert this or did you want approach it another way? Personally I'm fine to revert (and like I said, I think the original was more correct anyways) |
I was thinking reverting was the right thing, I just didn't get to it yesterday. Was planning to do so today unless you want to beat me to it. |
Closes #764
For call chains, we do some ~finicky logic for cases where the first item in a call chain is an expression like an array/hash literal. This is to accommodate some common use cases where something like calling
.freezeor.map(&:foo)after a big multiline expression. However, if there's comments in the middle of all this, we'll still need to fully multiline it, but the Ripper implementation multilines if there's comments anywhere in call chain's tokens, whereas the Prism one only was looking if they're between the end of the first expression and the first dot operator.(For the record, I think the Prism logic is probably more correct, but I'm fine with shooting for as much consistency with Ripper as possible for now, and we can explicitly choose this later.)