Group first arg for inline functions #947

Merged
merged 3 commits into from Mar 18, 2017

Conversation

Projects
None yet
3 participants
@Royce
Contributor

Royce commented Mar 8, 2017

A more restricted heuristic to address #888. Allows:

setTimeout(function() {
  thing();
}, 500);

(and other literals and identifiers) but not,

func(function() {
  thing();
}, true, false); // too many args

func(() => {
  thing();
}, {yes: true, cats: 5}); // too complex
src/printer.js
@@ -1952,6 +1952,23 @@ function shouldGroupLastArg(args) {
(!penultimateArg || penultimateArg.type !== lastArg.type);
}
+function shouldGroupFirstArg(args) {
+ if (args.length != 2) return false;

This comment has been minimized.

@vjeux

vjeux Mar 8, 2017

Collaborator

!== and please use {}

@vjeux

vjeux Mar 8, 2017

Collaborator

!== and please use {}

src/printer.js
+ return (firstArg.type === 'FunctionExpression' ||
+ (firstArg.type === 'ArrowFunctionExpression' &&
+ firstArg.body.type === 'BlockStatement')) &&
+ // Only allow one simple trailing arg.

This comment has been minimized.

@vjeux

vjeux Mar 8, 2017

Collaborator

I think that this is going to be very confusing.

timeout is good but this.timeout, 1000 * timeout, this.getTimeout(), this.props.timeout * 1000 is not good.

I'm not really sure what the best solution is but I figured I would give you some ways people are doing it from the fb codebase.

@vjeux

vjeux Mar 8, 2017

Collaborator

I think that this is going to be very confusing.

timeout is good but this.timeout, 1000 * timeout, this.getTimeout(), this.props.timeout * 1000 is not good.

I'm not really sure what the best solution is but I figured I would give you some ways people are doing it from the fb codebase.

This comment has been minimized.

@Royce

Royce Mar 9, 2017

Contributor

I'm trying to keep to James' request in #889 that it be "primitive" (literal or identifier).

I think it should allow MemberExpressions that only contain identifiers e.g. this.prop.timeout.

The binary expression timeout * 1000 should be disallowed because it's 2 things not one. (This doesn't look too bad split onto a seperate line, to my eye. Subjective!)

CallExpressions are interesting. this.props.getTimeout() doesn't look to bad, but if we are choosing between this.timeout, this.getTimeout() and this.getTimeout(args), I think it's less surprising to exclude all CallExpressions than exclude CallExpressions with args.

@Royce

Royce Mar 9, 2017

Contributor

I'm trying to keep to James' request in #889 that it be "primitive" (literal or identifier).

I think it should allow MemberExpressions that only contain identifiers e.g. this.prop.timeout.

The binary expression timeout * 1000 should be disallowed because it's 2 things not one. (This doesn't look too bad split onto a seperate line, to my eye. Subjective!)

CallExpressions are interesting. this.props.getTimeout() doesn't look to bad, but if we are choosing between this.timeout, this.getTimeout() and this.getTimeout(args), I think it's less surprising to exclude all CallExpressions than exclude CallExpressions with args.

src/printer.js
@@ -1955,6 +1955,15 @@ function shouldGroupLastArg(args) {
(!penultimateArg || penultimateArg.type !== lastArg.type);
}
+function isMemberExpressionWithIdentifierProperty(node) {

This comment has been minimized.

@Royce

Royce Mar 9, 2017

Contributor

This is a hideous function name. Help?!

@Royce

Royce Mar 9, 2017

Contributor

This is a hideous function name. Help?!

@Royce

This comment has been minimized.

Show comment
Hide comment
@Royce

Royce Mar 9, 2017

Contributor

Added support for this and this.something etc.

func(function() {
   thing();
 }, this.props.timeout);
 
 func((that) => {
   thing();
 }, this);
Contributor

Royce commented Mar 9, 2017

Added support for this and this.something etc.

func(function() {
   thing();
 }, this.props.timeout);
 
 func((that) => {
   thing();
 }, this);
@jlongster

This comment has been minimized.

Show comment
Hide comment
@jlongster

jlongster Mar 17, 2017

Member

I'm sorry we haven't followed up on this @Royce ! I talked to @vjeux about this earlier this week and we're not sure anymore what the right solution is. He's concerned that there are still lots of places that use an expression like timeout * 1000. I see that point, but not sure I want to go back to your previous PR of making it for work for anything... You usually only see this kind of code with a few functions like setTimeout.

If you all really want this, I suppose we could relax it to accept any expression but only do this if there are 2 args? I can't remember the history and if we already talked about that... But part of my concerns is definitely adding more args on the same line as the last brace so that would reduce those cases.

Member

jlongster commented Mar 17, 2017

I'm sorry we haven't followed up on this @Royce ! I talked to @vjeux about this earlier this week and we're not sure anymore what the right solution is. He's concerned that there are still lots of places that use an expression like timeout * 1000. I see that point, but not sure I want to go back to your previous PR of making it for work for anything... You usually only see this kind of code with a few functions like setTimeout.

If you all really want this, I suppose we could relax it to accept any expression but only do this if there are 2 args? I can't remember the history and if we already talked about that... But part of my concerns is definitely adding more args on the same line as the last brace so that would reduce those cases.

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Mar 17, 2017

Collaborator

I'm down with allowing it for two args. We should make sure that the second arg is not one that would trigger the last argument expansion.

Collaborator

vjeux commented Mar 17, 2017

I'm down with allowing it for two args. We should make sure that the second arg is not one that would trigger the last argument expansion.

@jlongster

This comment has been minimized.

Show comment
Hide comment
@jlongster

jlongster Mar 17, 2017

Member

Ok, sounds like a good compromise. Sorry for the roundabout @Royce. If you want one of us can finish this.

Member

jlongster commented Mar 17, 2017

Ok, sounds like a good compromise. Sorry for the roundabout @Royce. If you want one of us can finish this.

@Royce

This comment has been minimized.

Show comment
Hide comment
@Royce

Royce Mar 18, 2017

Contributor

Not a problem. I'll update/finish this later this weekend.

Contributor

Royce commented Mar 18, 2017

Not a problem. I'll update/finish this later this weekend.

@Royce

This comment has been minimized.

Show comment
Hide comment
@Royce

Royce Mar 18, 2017

Contributor

Ok, this is updated to accept (as the second arg) anything except one that would trigger the last argument expansion. But I still need to deal with {} and []. E.g. reduce((a, b) => { return a.push(b)}, []).

This got me thinking. Should an empty [] group as a last arg?

func(one, two, three, four, five, six, seven, eight, nine, ten, eleven, args, []);

->

func(one, two, three, four, five, six, seven, eight, nine, ten, eleven, args, [
]);

Seems weird.

Contributor

Royce commented Mar 18, 2017

Ok, this is updated to accept (as the second arg) anything except one that would trigger the last argument expansion. But I still need to deal with {} and []. E.g. reduce((a, b) => { return a.push(b)}, []).

This got me thinking. Should an empty [] group as a last arg?

func(one, two, three, four, five, six, seven, eight, nine, ten, eleven, args, []);

->

func(one, two, three, four, five, six, seven, eight, nine, ten, eleven, args, [
]);

Seems weird.

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Mar 18, 2017

Collaborator

Agreed, [] and {} should likely be treated the same

Collaborator

vjeux commented Mar 18, 2017

Agreed, [] and {} should likely be treated the same

Royce Townsend
Group first arg. Allow second arg to be empty object/array.
This implementation has a side effect of disallowing empty objects/arrays in the should group last arg heuristic.
@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Mar 18, 2017

Collaborator

Awesome! I'm happy with it! If you don't plan any more changes I'm going to merge it

Collaborator

vjeux commented Mar 18, 2017

Awesome! I'm happy with it! If you don't plan any more changes I'm going to merge it

@Royce

This comment has been minimized.

Show comment
Hide comment
@Royce

Royce Mar 18, 2017

Contributor

I'm good.

Contributor

Royce commented Mar 18, 2017

I'm good.

@vjeux vjeux merged commit 9eb389b into prettier:master Mar 18, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Mar 18, 2017

Collaborator

Thanks a lot!

Collaborator

vjeux commented Mar 18, 2017

Thanks a lot!

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