Skip to content

Conversation

@reese
Copy link
Collaborator

@reese reese commented Jan 6, 2026

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 .freeze or .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.)

Copy link
Collaborator

@froydnj froydnj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't mind me, I'm just completely misdiagnosing the issues here. 🤦‍♂️

Thanks for this fix!

@reese reese merged commit 1588924 into trunk Jan 6, 2026
8 checks passed
@reese reese deleted the reese-array-multilining branch January 6, 2026 13:58
@froydnj
Copy link
Collaborator

froydnj commented Jan 6, 2026

🤦‍♂️ 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.

@reese
Copy link
Collaborator Author

reese commented Jan 6, 2026

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.

@reese
Copy link
Collaborator Author

reese commented Jan 7, 2026

@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)

@froydnj
Copy link
Collaborator

froydnj commented Jan 7, 2026

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[prism] less indentation on array/hash literal calls with blocks

3 participants