Skip to content

Make curried arrow function and normal arrow function consistent#14633

Merged
fisker merged 47 commits intoprettier:mainfrom
seiyab:14290-consintent-bracket-around-ternary-in-curried-arrow
Apr 27, 2023
Merged

Make curried arrow function and normal arrow function consistent#14633
fisker merged 47 commits intoprettier:mainfrom
seiyab:14290-consintent-bracket-around-ternary-in-curried-arrow

Conversation

@seiyab
Copy link
Copy Markdown
Collaborator

@seiyab seiyab commented Mar 30, 2023

Description

Resolves #14290 .

What I did are...

  • unify printArrowChain() into printArrowFunction() to fix the issue and prevent other existing / future inconsistencies.
  • extract some codes from printArrowFunction() (and printArrowChain()) into new mayBreakAfterShortPrefix(), joinArrowFunctionSignatures() and printArrowBody().

I'm worrying that it might get somewhat harder to read and change.
But I don't have better ideas.

Old contents. Just for see history

Still in progress.
I think I need to

  • check similar bugs around here
  • check consistency
  • refactor it to prevent similar bugs
  • add more tests

Edit: I'm busier than before. Go ahead if someone can go faster, and feel free to refer to or cherry pick my commits. I myself am still working on it.
Edit 2: I'm still working on it. I'm checking consistency more.

Checklist

  • I’ve added tests to confirm my change works.
  • (If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory).
  • (If the change is user-facing) I’ve added my changes to changelog_unreleased/*/XXXX.md file following changelog_unreleased/TEMPLATE.md.
  • I’ve read the contributing guidelines.

Try the playground for this PR

@seiyab seiyab changed the title WIP: make brackets around ternary as arrow function body consistent WIP: make curried arrow function and normal arrow function consistent Apr 18, 2023
@seiyab seiyab changed the title WIP: make curried arrow function and normal arrow function consistent Make curried arrow function and normal arrow function consistent Apr 18, 2023
@seiyab seiyab marked this pull request as ready for review April 18, 2023 13:10
@fisker
Copy link
Copy Markdown
Member

fisker commented Apr 20, 2023

@seiyab I've made changes I want. Please take a look, and feel free to make changes if you want.

@fisker
Copy link
Copy Markdown
Member

fisker commented Apr 20, 2023

arrow-function.js even longer than function.js 😄

@fisker
Copy link
Copy Markdown
Member

fisker commented Apr 20, 2023

I didn't know curried-arrow-functions not calling print before, so the cursor tracking and range format support should be very bad. But it's out scope of this PR.

@fisker fisker added this to the 3.0 milestone Apr 20, 2023
@seiyab
Copy link
Copy Markdown
Collaborator Author

seiyab commented Apr 20, 2023

I'll take a look.
I also intended to suggest splitting the file as another PR 😄 .

Comment on lines +61 to +62
const isArrowFunctionChain = (node) =>
node.body.type === "ArrowFunctionExpression";
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Question]
As of main, it looked to currentNode.body.type !== "ArrowFunctionExpression" || args.expandLastArg reading just rec() in printArrowFunction().
Do the difference cause a problem?

Though it doesn't cause any problem as far as I tried, I'm not sure about it because I'm not familiar with expandLastArg.
Example of input I tried:

fooooooooooooooooooooooo(action => a => b => 
  (dispatch123456789098765432123451234567789(action), 1, 2)
);

Copy link
Copy Markdown
Member

@fisker fisker Apr 20, 2023

Choose a reason for hiding this comment

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

Do the difference cause a problem?

I also worried about it, and it may cause problem, but I forgot about it...

Related logic here

function shouldExpandLastArg(args, argDocs, options) {

We need check that.

My idea is get things out of rec(), so we don't need pass many arguments around.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

As my first impression, removing || args.expandLastArg int rec() looked to make sense.
But it will change some results, and previous format looks slightly better.
I'm still investigating.

- request.get(
-   "https://preview-9992--prettier.netlify.app",
-   (head) =>
-     (body) =>
-     (modyLoremIpsumDolorAbstractProviderFactoryServiceModule) => {
-       console.log(head, body);
-     },
- );
+ request.get("https://preview-9992--prettier.netlify.app", (head) =>
+   (body) =>
+   (modyLoremIpsumDolorAbstractProviderFactoryServiceModule) => {
+     console.log(head, body);
+   });

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This yields same results regardless of || args.expandLastArg. So expandLastArg looks to work.

request.get("https://preview-9992--prettier.netlify.app", (head) => (body) => {
  console.log(head, body);
})

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I read the code again, when expandLastArg is true. functionBody can be an ArrowFunctionExpression, and functionBody decides how the body should be printed. Can't find a case to test. 98c36c2 (#14633) should restore the old behavior, I think.

Copy link
Copy Markdown
Member

@fisker fisker Apr 21, 2023

Choose a reason for hiding this comment

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

The following code can make functionBody be an ArrowFunctionExpression and effect shouldBreakChain

a("", "", ({}) => () => {
  a;
});
  • last argument is ArrrowFunctionExpression satisfies couldExpandArg
  • first two arguments satisfies args.length !== 2 in shouldExpandLastArg
  • first two arguments is very simple so they will pass headArgs.some(willBreak) in printCallArguments
  • the function parameter is not Identifier, so it effect shouldBreakChain

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Finally find a case formats differently....

a("", "", ({}) => () => (a ? b : c));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

# this one
>node bin/prettier test.js
a(
  "",
  "",
  ({}) =>
    () => (a ? b : c),
);

# stable
>node node_modules/prettier/bin-prettier.js test.js
a(
  "",
  "",
  ({}) =>
    () =>
      a ? b : c
);

Copy link
Copy Markdown
Collaborator Author

@seiyab seiyab left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@fisker
Copy link
Copy Markdown
Member

fisker commented Apr 21, 2023

@seiyab Can you make the following code work as v2?

a(
  "",
  "",
  ({}) =>
    () =>
    () =>
    () =>
    () =>
    () =>
    () => (a ? b : c),
);

@seiyab
Copy link
Copy Markdown
Collaborator Author

seiyab commented Apr 21, 2023

I'll take a look. Thanks a lot for your detailed investigation.

@seiyab
Copy link
Copy Markdown
Collaborator Author

seiyab commented Apr 21, 2023

I don't understand it yet, but perhaps, we can say, current revision's behavior shows more consistent result with non-curried arrow function?

a(
  "",
  "",
  ({}) =>
    () =>
    () =>
    () =>
    () =>
    () =>
    () => (a ? b : c),
);

a(
  "",
  "",
  (
    {},
    argument,
    argument,
    argument,
    argument,
    argument,
    argument,
    argument,
    argument,
  ) => (a ? b : c),
);

@fisker
Copy link
Copy Markdown
Member

fisker commented Apr 21, 2023

But we put body on next line if it's an identifier

Copy link
Copy Markdown
Member

@fisker fisker left a comment

Choose a reason for hiding this comment

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

Good job.

@seiyab
Copy link
Copy Markdown
Collaborator Author

seiyab commented Apr 24, 2023

Thank you for your review.
To be honest, I'm worrying that 0eb4cc7 might not truly meet your request.
Anyway, I investigated the input and make ternary be printed same as identifier (and many other "default" ones).

(I don't mean to stop merging as long as you accept it. This comment is for information only.)

@fisker
Copy link
Copy Markdown
Member

fisker commented Apr 27, 2023

We can always fix bugs when they are found.

@fisker fisker merged commit 7f64e92 into prettier:main Apr 27, 2023
@seiyab seiyab deleted the 14290-consintent-bracket-around-ternary-in-curried-arrow branch May 1, 2023 22:58
medikoo pushed a commit to medikoo/prettier-elastic that referenced this pull request Feb 15, 2024
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.

Brackets around ternary as twice-curried arrow function body are removed

2 participants