method chain breaking heuristic#6685
Conversation
|
Hi! Thanks for taking the time to experiment with this! I don’t think using a regex is going to be a sustainable solution. Isn’t it unexpected that the following two lines are formatted differently? Prettier pr-6685 --parser babylonInput: name.toLowerCase().replace("hi", "hello").trim();
name.toLowerCase().replace("hi!", "hello").trim();Output: name.toLowerCase().replace("hi", "hello").trim();
name
.toLowerCase()
.replace("hi!", "hello")
.trim(); |
|
@lydell agreed, am working on working on replacing the regex with a function that makes sure the arguments are in a whitelist. Edit: have now updated to use a function. |
|
This looks really nice, excited to see a viable solution for this! |
|
@mmkal What’s the status on this one? |
|
@lydell just awaiting feedback - then I need to add to the changelog, but that's about it. Was there something you think I should add/remove to the heuristic? |
|
Cool, I wasn’t aware you were finished. We’ll need to review then. |
|
Looking forward to this 😍 |
| db.branch( | ||
| db | ||
| .table("users") | ||
| .filter({ email }) | ||
| .count(), | ||
| db.table("users").insert({ email }), | ||
| db.table("users").filter({ email }) | ||
| ); |
There was a problem hiding this comment.
This inconsistency is somewhat unexpected to me.
There was a problem hiding this comment.
Overall, the decision to exclude the last chain member from the isSimple check looks questionable to me. Also I think simple shortcut object literals like { email } should pass this check.
There was a problem hiding this comment.
Good point about object literals with shorthand properties, will add them. I'll also see what difference including the last chain member makes, I'm not too tied to it being all but one. I made it that way to start with because the chains can still be pretty readable, but I don't see it making a huge difference.
| a | ||
| .b() | ||
| .c() | ||
| // Comment | ||
| ?.d() | ||
| a.b().c()// Comment | ||
| ?.d() |
There was a problem hiding this comment.
Should this still be broken onto multiple lines? This doesn’t look very good, especially the missing space before the inline comment.
There was a problem hiding this comment.
Ah, thought I had addressed this. Will push a fix shortly
There was a problem hiding this comment.
Yeah, as I said I missed it. Thought I'd fixed but must have been looking in the wrong spot. I'm doing this outside of work so will @ you when it's fixed.
There was a problem hiding this comment.
@j-f1 I did some digging, and it might be that this PR has unearthed a pre-existing issue, since the same bug can be reproduced by just changing the break length from 3 to 4: master...mmkal:optional-chaining-comment-bug . That said, I'll look into this more anyway - if it wasn't possible before and now is, it's a regression even if the code causing it has always been there.
|
@mmkal Two more things.
|
|
I excluded unaries because I would want non-simple things like Will move to utils and rename this weekend. |
|
I think !x and -1 should be considered simple. |
bc85c8a to
605c894
Compare
ba9580d to
a11836f
Compare
|
@thorn0 @lydell @sosukesuzuki anything left on this I need to do to get it merged? I see the 2.0 label has been added, does that mean it won't go into master until January? I'm a little worried about merge conflicts. |
a11836f to
34cfd82
Compare
e16f758 to
7d88fb8
Compare
|
/cc @thorn0 please do not merge PR in future without minimum 3 approved, especial on this big changes, you can ping any if approves is not enough, thanks |
|
@evilebottnawi I believe you can update the branch protection settings to mandate minimum 3 approvals. |
Addresses #3107
A more opinionated alternative to #4765; this solution doesn't depend on the formatting of the input. Instead it uses a heuristic for the complexity of the arguments in the chained method call.
Right now, this heuristic is just a regex - if there are any non-whitelisted characters, prettier will break.The heuristic can be gradually improved, this is a starting point.I've included examples from #3107, #4765, #1565 and #4125 as tests. In the majority of cases, prettier is now doing what the commenter wanted - both those in the generally-prefer-multiline camp and the generally-prefer-one-line camp. I added the examples before making the prettier changes, so you can view the diff here: 613d83e?w=1
It's still not perfect, of course. But I've been on teams which have done each one of
// prettier-ignoresAll of which lead to bikeshedding on PRs and ugly code, the two keys things prettier is designed to avoid.
@sharmilajesupaul @j-f1 @SimenB @RyanZim @azz would be interested to hear your thoughts on this.
CHANGELOG.unreleased.mdfile following the template.✨Try the playground for this PR✨