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.

Sign up for GitHub

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

Jump to bottom

Hug object types in function parameters in Flow like in TS #13396

Merged
merged 5 commits into from
Aug 30, 2022
Merged

Hug object types in function parameters in Flow like in TS #13396

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
86808ad
Hug object types in function parameters in Flow like in TS
thorn0 Aug 28, 2022
6c1b60d
Unify rest param printing with TS
thorn0 Aug 28, 2022
63d43e2
Add same test for TS
thorn0 Aug 29, 2022
4036178
Add changelog
thorn0 Aug 29, 2022
f0cd175
Remove number check and rename function
thorn0 Aug 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/language-js/print/function-parameters.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,8 @@ function shouldHugFunctionParameters(node) {
parameter.typeAnnotation.type === "TSTypeAnnotation") &&
isObjectType(parameter.typeAnnotation.typeAnnotation)) ||
(parameter.type === "FunctionTypeParam" &&
isObjectType(parameter.typeAnnotation)) ||
isObjectType(parameter.typeAnnotation) &&
parameter !== node.rest) ||
(parameter.type === "AssignmentPattern" &&
(parameter.left.type === "ObjectPattern" ||
parameter.left.type === "ArrayPattern") &&
Expand Down
44 changes: 24 additions & 20 deletions src/language-js/print/object.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,12 @@ import {
getComments,
CommentCheckFlags,
isNextLineEmpty,
isObjectType,
} from "../utils/index.js";
import { locStart, locEnd } from "../loc.js";

import { printOptionalToken, printTypeAnnotation } from "./misc.js";
import { shouldHugFunctionParameters } from "./function-parameters.js";
import { shouldHugType } from "./type-annotation.js";
import { printHardlineAfterHeritage } from "./class.js";

/** @typedef {import("../../document/builders.js").Doc} Doc */
Expand Down Expand Up @@ -201,26 +201,21 @@ function printObject(path, options, print) {
if (
path.match(
(node) => node.type === "ObjectPattern" && !node.decorators,
(node, name, number) =>
shouldHugFunctionParameters(node) &&
(name === "params" ||
name === "parameters" ||
name === "this" ||
name === "rest") &&
number === 0
) ||
path.match(
shouldHugType,
(node, name) => name === "typeAnnotation",
(node, name) => name === "typeAnnotation",
(node, name, number) =>
shouldHugFunctionParameters(node) &&
(name === "params" ||
name === "parameters" ||
name === "this" ||
name === "rest") &&
number === 0
shouldHugParameter
) ||
(isObjectType(node) &&
(path.match(
undefined,
(node, name) => name === "typeAnnotation",
(node, name) => name === "typeAnnotation",
shouldHugParameter
) ||
path.match(
undefined,
(node, name) =>
node.type === "FunctionTypeParam" && name === "typeAnnotation",
shouldHugParameter
))) ||
Copy link
Member Author

Choose a reason for hiding this comment

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

Would be cool if path.match supported alternatives like this:

path.match(
  isObjectType,
  [
    [
      (node, name) => name === "typeAnnotation",
      (node, name) => name === "typeAnnotation"
    ],
    [
      (node, name) =>
        node.type === "FunctionTypeParam" && name === "typeAnnotation"
    ]
  ],
  shouldHugParameter
)

// Assignment printing logic (printAssignment) is responsible
// for adding a group if needed
(!shouldBreak &&
Expand All @@ -237,4 +232,13 @@ function printObject(path, options, print) {
return group(content, { shouldBreak });
}

function shouldHugParameter(node, name, number) {
return (
((number === 0 && (name === "params" || name === "parameters")) ||
name === "this" ||
name === "rest") &&
shouldHugFunctionParameters(node)
Copy link
Member

@fisker fisker Aug 30, 2022 •
edited

Choose a reason for hiding this comment

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

If function has .this and normal parameters, won't this pass on second parameter?

Copy link
Member

Choose a reason for hiding this comment

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

Use getFunctionParameters().indexOf?

Copy link
Member Author

Choose a reason for hiding this comment

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

shouldHugFunctionParameters checks that there is only one parameter, but yes, doing what you propose would be easier to follow.

Copy link
Member

Choose a reason for hiding this comment

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

If shouldHugFunctionParameters only pass on one parameter. The number check is redundant. Only need make sure it's a parameter.

Copy link
Member

Choose a reason for hiding this comment

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

I'm a little bit confused, the "node" is a function, right? It can't be another function's parameter, how name can be "parameters"? The "name" is key of child?

Copy link
Member

Choose a reason for hiding this comment

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

Ah yes, this is different from callback of AstPath#each, the name is key of node there.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, getFunctionParameters().indexOf looks clumsy because I need to pass a node to it, and to get this node I have to use name and number anyway. So I'm going to simply remove the number check.

Copy link
Member Author

Choose a reason for hiding this comment

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

I also renamed shouldHugFunctionParameters to shouldHugTheOnlyFunctionParameter to make it clear that it checks that there is only one parameter.

);
}

export { printObject };
42 changes: 42 additions & 0 deletions tests/format/flow/function-single-destructuring/__snapshots__/jsfmt.spec.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,3 +133,45 @@ const X = (props: { a: boolean }) => <A />;

================================================================================
`;

exports[`object-type-in-declare-function.js format 1`] = `
====================================options=====================================
parsers: ["flow", "babel"]
printWidth: 80
| printWidth
=====================================input======================================
declare function foo(this: { a: boolean, b: string, c: number }):
Promise<Array<foo>>

declare function bazFlip({ a: boolean, b: string, c: number }):
Promise<Array<foo>>

declare function bar(...{ a: boolean, b: string, c: number }):
Promise<Array<foo>>

declare function bar(...x: { a: boolean, b: string, c: number }):
Promise<Array<foo>>

=====================================output=====================================
declare function foo(this: {
a: boolean,
b: string,
c: number,
}): Promise<Array<foo>>;

declare function bazFlip({
a: boolean,
b: string,
c: number,
}): Promise<Array<foo>>;

declare function bar(
...{ a: boolean, b: string, c: number }
): Promise<Array<foo>>;

declare function bar(
...x: { a: boolean, b: string, c: number }
): Promise<Array<foo>>;

================================================================================
`;
11 changes: 11 additions & 0 deletions tests/format/flow/function-single-destructuring/object-type-in-declare-function.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
declare function foo(this: { a: boolean, b: string, c: number }):
Promise<Array<foo>>

declare function bazFlip({ a: boolean, b: string, c: number }):
Promise<Array<foo>>

declare function bar(...{ a: boolean, b: string, c: number }):
Promise<Array<foo>>

declare function bar(...x: { a: boolean, b: string, c: number }):
Promise<Array<foo>>
42 changes: 42 additions & 0 deletions tests/format/typescript/declare/__snapshots__/jsfmt.spec.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,3 +215,45 @@ class C {

================================================================================
`;

exports[`object-type-in-declare-function.ts format 1`] = `
====================================options=====================================
parsers: ["typescript"]
printWidth: 80
| printWidth
=====================================input======================================
declare function foo(this: { a: boolean, b: string, c: number }):
Promise<Array<foo>>

declare function bazFlip({ a: boolean, b: string, c: number }):
Promise<Array<foo>>

declare function bar(...{ a: boolean, b: string, c: number }):
Promise<Array<foo>>

declare function bar(...x: { a: boolean, b: string, c: number }):
Promise<Array<foo>>

=====================================output=====================================
declare function foo(this: {
a: boolean;
b: string;
c: number;
}): Promise<Array<foo>>;

declare function bazFlip({
a: boolean,
b: string,
c: number,
}): Promise<Array<foo>>;

declare function bar(
...{ a: boolean, b: string, c: number }
): Promise<Array<foo>>;

declare function bar(
...x: { a: boolean; b: string; c: number }
): Promise<Array<foo>>;

================================================================================
`;
11 changes: 11 additions & 0 deletions tests/format/typescript/declare/object-type-in-declare-function.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
declare function foo(this: { a: boolean, b: string, c: number }):
Promise<Array<foo>>

declare function bazFlip({ a: boolean, b: string, c: number }):
Promise<Array<foo>>

declare function bar(...{ a: boolean, b: string, c: number }):
Promise<Array<foo>>

declare function bar(...x: { a: boolean, b: string, c: number }):
Promise<Array<foo>>