Skip to content
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

Function call wrapping #123

Merged
merged 3 commits into from
Mar 5, 2018
Merged

Function call wrapping #123

merged 3 commits into from
Mar 5, 2018

Conversation

czosel
Copy link
Collaborator

@czosel czosel commented Feb 25, 2018

No description provided.

@czosel czosel changed the title Function call wrapping (WIP) Function call wrapping Feb 25, 2018

$this->something->method($argument, $this->more->stuff($this->even->more->
things->
complicatedMethod()));
Copy link
Collaborator Author

@czosel czosel Feb 25, 2018

Choose a reason for hiding this comment

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

I tried using a conditionalGroup to wrap function calls just like the JS printer does - which works generally quite well, but I'm still having issues with this nested call example. I think it should be formatted like this:

$this->something->method($argument, $this->more->stuff(
  $this->even->more->things->complicatedMethod()
));

Maybe @azz can point me in the right direction?

Copy link
Contributor

Choose a reason for hiding this comment

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

@czosel do you know where the JS printer does this? I'm mostly just curious

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

🙏

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@mgrip mgrip left a comment

Choose a reason for hiding this comment

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

Still some cases to work through, but this looks sooo much better

@@ -108,6 +116,30 @@ function printLines(path, options, print) {
return join(hardline, printed);
}

function printArgumentsList(path, options, print) {
Copy link
Contributor

Choose a reason for hiding this comment

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

mind just adding a comment outlining the overall strategy here?


$this->something->method($argument, $this->more->stuff($this->even->more->
things->
complicatedMethod()));
Copy link
Contributor

Choose a reason for hiding this comment

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

@czosel
Copy link
Collaborator Author

czosel commented Mar 2, 2018

Removing mediumForm actually also yields quite good results. I'll now look into using mediumForm only for arrayExpressions. If you want we can merge this state already, since it's already much better than the current one.

@czosel
Copy link
Collaborator Author

czosel commented Mar 2, 2018

@mgrip @evilebottnawi I think this is ready for review now 🎉

@czosel czosel changed the title (WIP) Function call wrapping Function call wrapping Mar 2, 2018
@czosel czosel mentioned this pull request Mar 3, 2018
Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Great work 👍

@czosel czosel merged commit be5a4ea into prettier:master Mar 5, 2018
@czosel czosel deleted the nested-functions branch March 5, 2018 20:58
@mgrip
Copy link
Contributor

mgrip commented Mar 6, 2018

This is great! Nice work @czosel 💯

@harmenjanssen
Copy link

Hi! Sorry to be leaving a comment on such an old PR, but I started using this plugin today, and noticed the following format being rendered by Prettier:

array_map(function ($entry) {
    return $entry * 2;
}, $arr);

Through the snapshots I was able to find PR #23, in which a change is pushed that changes the above to this:

array_map(
  function ($entry) {
    return $entry * 2;
  },
  $arr
);

However, about a month after that PR @czosel, the author of both PRs, pushes the changes in this PR reverting that beautiful format to the old.

I'm wondering: is this deliberate? If not, would you accept a PR that changes the format to that in the second snippet.
I can take a stab at it, but maybe there's a reasoning why the above snippet is preferred.

@czosel
Copy link
Collaborator Author

czosel commented Sep 19, 2020

Hi @harmenjanssen!

I think this was not deliberate. In #23 we optimized the printing of long function call expressions, but made the printing of expressions like the one you posted above a little worse, which was an acceptable trade-off at the time. If you find a clever way to optimize the example above that also works for long call expressions, I'd be happy to merge your PR 👍

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.

None yet

4 participants