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

Expand first argument if it is an inline function #889

Closed
wants to merge 1 commit into from

Conversation

Royce
Copy link
Contributor

@Royce Royce commented Mar 4, 2017

Addresses #888

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

No longer produces.

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

@vjeux
Copy link
Contributor

vjeux commented Mar 4, 2017

The examples look really good to me, cc @robarnold who wanted this as well.

src/printer.js Outdated
@@ -1952,6 +1952,14 @@ function shouldGroupLastArg(args) {
(!penultimateArg || penultimateArg.type !== lastArg.type);
}

function shouldGroupFirstArg(args) {
const firstArg = args[0];
Copy link
Contributor

Choose a reason for hiding this comment

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

you need to check that there is at least one argument, otherwise it's going to throw an exception Cannot read property 'type' of undefined

return (firstArg.type === 'FunctionExpression' ||
(firstArg.type === 'ArrowFunctionExpression' &&
firstArg.body.type === 'BlockStatement')) &&
args.slice(1).every(a => !typeIsFunction(a.type));
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

src/printer.js Outdated
concat(["(", join(concat([", "]), printed), ")"]),
concat([
"(",
group(printed[0], { shouldBreak: true }),
Copy link
Contributor

Choose a reason for hiding this comment

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

it looks like this part is the only difference. Could you try to merge the two instead of copy pasting everything?

Something like that (feel free to find better variable names:

const groupFirst = shouldGroupFirst()
const groupLast = shouldGroupLast()

if (groupFirst || groupLast) {
  const begin = groupFirst ? printed[0] : printed.slice(0, -1);
  const end = groupFirst ? printed.slice(1) : util.getLast(printed);

  // ...
}

@Royce
Copy link
Contributor Author

Royce commented Mar 4, 2017

@vjeux Thanks for the prompt feedback. I've updated the PR

@vjeux
Copy link
Contributor

vjeux commented Mar 4, 2017

Thanks, the code looks good to me. Next step is to wait for @jlongster to decide if we want to add this behavior or not :)

@jlongster
Copy link
Member

My gut tells me not to do this. It's just getting too specific. At some point we have to draw the line and say we aren't going to specialize any more patterns because it makes it too easy to become inconsistent.

Once you starting adding more params or doing more things it becomes unreadable:

setInterval(function() {
  thing();
}, short ? 200 : 500);

func(function() {
  thing();
}, { param: " foo" });

All of these are much better to expand all of the arguments. We shouldn't violate that rule too much. I think the above code looks weird to you only because you are used to see setTimeout specifically formatted that way. Once you use prettier for a while and expect it to break arguments if they span multiple lines, the output is expected and imho a lot more consistent with the rest of the program.

@Royce
Copy link
Contributor Author

Royce commented Mar 7, 2017

I see your point. Thanks for your time, James, and for a great program :)

@rhengles
Copy link
Contributor

rhengles commented Mar 7, 2017

@Royce Implemented in arijs@8d18e1a as groupFirstArg option

@jlongster
Copy link
Member

@Royce I think I might be OK with this if it's a lot more strict. Only do it if there are only 2 parameters and the second one is "primitive", meaning it's a literal or an identifier. I tend to resist adding a lot more heuristics but the fact is prettier is really nice because of these kinds of heuristics.

If want to re-open a PR with those changes, I think we can merge it.

@Royce
Copy link
Contributor Author

Royce commented Mar 8, 2017

@jlongster Please see #947 for a stricter heuristic for grouping first arg. :)

@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jan 21, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants