No parenthesis for Flow shorthand with one arg #972

Merged
merged 12 commits into from Mar 16, 2017

Conversation

Projects
None yet
2 participants
@yamafaktory
Contributor

yamafaktory commented Mar 9, 2017

Experimental PR trying to solve #969.

yamafaktory added some commits Mar 9, 2017

src/printer.js
@@ -2062,8 +2062,12 @@ function printFunctionParams(path, print, options) {
return concat(["(", join(", ", printed), ")"]);
}
+ const isFlowShorthandWithOneArg = fun.type === "FunctionTypeAnnotation" &&
+ parent.type === "ObjectTypeProperty" && fun.params.length === 1 &&

This comment has been minimized.

@yamafaktory

yamafaktory Mar 9, 2017

Contributor

Oh, I don't need that...

@yamafaktory

yamafaktory Mar 9, 2017

Contributor

Oh, I don't need that...

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Mar 9, 2017

Collaborator

Can you add test cases for:

type T = { method: (a) => void };
type T = { method(a): void };
declare class X { method(a): void }
declare function f(a): void;
var f: (a) => void;
Collaborator

vjeux commented Mar 9, 2017

Can you add test cases for:

type T = { method: (a) => void };
type T = { method(a): void };
declare class X { method(a): void }
declare function f(a): void;
var f: (a) => void;
@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Mar 9, 2017

Collaborator

Yeah, that's what I suspected, the "normal" solution completely breaks:

type T = { method: a => void };
type T = { methoda: void };
declare class X { methoda: void }
declare function fa: void;
var f: a => void;

Also, this is just a sample of all the ways you can combine this ast. I'm honestly super scared about breaking something here.

Collaborator

vjeux commented Mar 9, 2017

Yeah, that's what I suspected, the "normal" solution completely breaks:

type T = { method: a => void };
type T = { methoda: void };
declare class X { methoda: void }
declare function fa: void;
var f: a => void;

Also, this is just a sample of all the ways you can combine this ast. I'm honestly super scared about breaking something here.

@yamafaktory

This comment has been minimized.

Show comment
Hide comment
@yamafaktory

yamafaktory Mar 9, 2017

Contributor

Yeah I've just seen that... will try to see what I can do...

Contributor

yamafaktory commented Mar 9, 2017

Yeah I've just seen that... will try to see what I can do...

@yamafaktory

This comment has been minimized.

Show comment
Hide comment
@yamafaktory

yamafaktory Mar 9, 2017

Contributor

Checking the equality of the start of the fun and the one of the first param seems to fix it.

Contributor

yamafaktory commented Mar 9, 2017

Checking the equality of the start of the fun and the one of the first param seems to fix it.

yamafaktory added some commits Mar 9, 2017

@yamafaktory

This comment has been minimized.

Show comment
Hide comment
@yamafaktory

yamafaktory Mar 9, 2017

Contributor

Ok, this is now a regression for some other tests.

Contributor

yamafaktory commented Mar 9, 2017

Ok, this is now a regression for some other tests.

yamafaktory added some commits Mar 9, 2017

@yamafaktory

This comment has been minimized.

Show comment
Hide comment
@yamafaktory

yamafaktory Mar 9, 2017

Contributor

@vjeux I think I got something better 🤞. At least the tests are passing, but I maybe need more test cases, not quite sure about it.

Contributor

yamafaktory commented Mar 9, 2017

@vjeux I think I got something better 🤞. At least the tests are passing, but I maybe need more test cases, not quite sure about it.

@yamafaktory

This comment has been minimized.

Show comment
Hide comment
@yamafaktory

yamafaktory Mar 10, 2017

Contributor

@cpojer do you have some other examples coming in mind so that I can test my implementation against it?

Contributor

yamafaktory commented Mar 10, 2017

@cpojer do you have some other examples coming in mind so that I can test my implementation against it?

@vjeux

Sorry for the super late reviews, React Conf has been kind of crazy. If you can address the two comments, then it looks good to go. I really hope that it's not going to break anything else :)

Great work on tackling a non trivial thing!

src/printer.js
@@ -2062,8 +2062,12 @@ function printFunctionParams(path, print, options) {
return concat(["(", join(", ", printed), ")"]);
}
+ const isFlowShorthandWithOneArg = fun.type === "FunctionTypeAnnotation" &&

This comment has been minimized.

@vjeux

vjeux Mar 16, 2017

Collaborator

Could you use isObjectTypePropertyAFunction helper in order to do this check: https://github.com/prettier/prettier/blob/master/src/printer.js#L3064-L3072

This way we don't have random location comparison spread across the codebase.

@vjeux

vjeux Mar 16, 2017

Collaborator

Could you use isObjectTypePropertyAFunction helper in order to do this check: https://github.com/prettier/prettier/blob/master/src/printer.js#L3064-L3072

This way we don't have random location comparison spread across the codebase.

This comment has been minimized.

@yamafaktory

yamafaktory Mar 16, 2017

Contributor

Ok, done. But I needed to create a similar one called isTypeAnnotationAFunction and also keep a checking for TypeAlias too 😄.

@yamafaktory

yamafaktory Mar 16, 2017

Contributor

Ok, done. But I needed to create a similar one called isTypeAnnotationAFunction and also keep a checking for TypeAlias too 😄.

+
+declare function f(a): void;
+
+var f: (a) => void;

This comment has been minimized.

@vjeux

vjeux Mar 16, 2017

Collaborator

A few more things to test:

interface F { m(string): number };
interface F { m: (string) => number };
function f(o: { f: (string) => void }) {}
function f(o: { f(string): void }) {}
@vjeux

vjeux Mar 16, 2017

Collaborator

A few more things to test:

interface F { m(string): number };
interface F { m: (string) => number };
function f(o: { f: (string) => void }) {}
function f(o: { f(string): void }) {}

This comment has been minimized.

@yamafaktory

yamafaktory Mar 16, 2017

Contributor

Done and passing with my implementation!

@yamafaktory

yamafaktory Mar 16, 2017

Contributor

Done and passing with my implementation!

yamafaktory added some commits Mar 16, 2017

+// Hack to differentiate between the following two which have the same ast
+// declare function f(a): void;
+// var f: (a) => void;
+function isTypeAnnotationAFunction(node) {

This comment has been minimized.

@yamafaktory

yamafaktory Mar 16, 2017

Contributor

Similar to isObjectTypePropertyAFunction.

@yamafaktory

yamafaktory Mar 16, 2017

Contributor

Similar to isObjectTypePropertyAFunction.

@yamafaktory

This comment has been minimized.

Show comment
Hide comment
@yamafaktory

yamafaktory Mar 16, 2017

Contributor

@vjeux I've just noticed the build passed for v4 only and failed for no reason of the other two.

Contributor

yamafaktory commented Mar 16, 2017

@vjeux I've just noticed the build passed for v4 only and failed for no reason of the other two.

@@ -2062,8 +2062,12 @@ function printFunctionParams(path, print, options) {
return concat(["(", join(", ", printed), ")"]);
}
+ const isFlowShorthandWithOneArg = (isObjectTypePropertyAFunction(parent) ||
+ isTypeAnnotationAFunction(parent) || parent.type === "TypeAlias") &&
+ fun.params.length === 1 && fun.params[0].name === null && fun.rest === null;

This comment has been minimized.

@vjeux

vjeux Mar 16, 2017

Collaborator

Looks like this has less cases than arrow function:

        n.params.length === 1 &&
        !n.rest &&
        n.params[0].type === "Identifier" &&
        !n.params[0].typeAnnotation &&
        !n.params[0].leadingComments &&
        !n.params[0].trailingComments &&
        !n.params[0].optional &&
        !n.predicate &&
        !n.returnType

https://github.com/prettier/prettier/blob/master/src/printer.js#L304-L313

Can you add the following tests as well:

type f = (...arg) => void;
type f = (/* comment */ arg) => void;
type f = (arg /* comment */) => void;
type f = (?arg) => void;
@vjeux

vjeux Mar 16, 2017

Collaborator

Looks like this has less cases than arrow function:

        n.params.length === 1 &&
        !n.rest &&
        n.params[0].type === "Identifier" &&
        !n.params[0].typeAnnotation &&
        !n.params[0].leadingComments &&
        !n.params[0].trailingComments &&
        !n.params[0].optional &&
        !n.predicate &&
        !n.returnType

https://github.com/prettier/prettier/blob/master/src/printer.js#L304-L313

Can you add the following tests as well:

type f = (...arg) => void;
type f = (/* comment */ arg) => void;
type f = (arg /* comment */) => void;
type f = (?arg) => void;

This comment has been minimized.

@yamafaktory

yamafaktory Mar 16, 2017

Contributor

Ok, I will do it!

@yamafaktory

yamafaktory Mar 16, 2017

Contributor

Ok, I will do it!

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Mar 16, 2017

Collaborator

Looks like both builds got stuck downloading node after 50min. I just restarted them.

Collaborator

vjeux commented Mar 16, 2017

Looks like both builds got stuck downloading node after 50min. I just restarted them.

@yamafaktory

This comment has been minimized.

Show comment
Hide comment
@yamafaktory

yamafaktory Mar 16, 2017

Contributor

@vjeux done!

Contributor

yamafaktory commented Mar 16, 2017

@vjeux done!

@vjeux vjeux merged commit 915967b into prettier:master Mar 16, 2017

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Mar 16, 2017

Collaborator

Boom! Thanks a lot!

Collaborator

vjeux commented Mar 16, 2017

Boom! Thanks a lot!

@yamafaktory yamafaktory deleted the yamafaktory:969-flow-shorthand-one-arg-no-parens branch Mar 16, 2017

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