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

Simple arrow function parens option #3324

Merged
merged 5 commits into from Nov 28, 2017

Conversation

Projects
None yet
8 participants
@suchipi
Member

suchipi commented Nov 26, 2017

This is based on #2676 by @rattrayalex. Originally, I was going to rebase that branch and make changes there, but rebasing proved difficult due to unrelated changes in master.

This PR adds an "--arrow-function-parentheses" choice option with two options: "always" and "avoid". Avoid is the same as now (try not to print them if you can). Always is the new option (always print them). Avoid is the default, for backwards-compatibility.

Unlike #2676, this does not include the callbacks option AKA functional. Even though I find it interesting, I don't think there is community/collaborator buy-in or consensus, and I don't want to not add "always" because of that.

This PR also includes a fix that @rattrayalex started in his branch to inline type parameters on test callback functions (it("very long description", <T>(done) => { /* ... */ })).

@suchipi suchipi requested a review from rattrayalex Nov 26, 2017

@suchipi suchipi referenced this pull request Nov 26, 2017

Closed

Arrow parens option #2676

@@ -96,6 +96,19 @@ Put the `>` of a multi-line JSX element at the end of the last line instead of b
| ------- | ------------------------- | ---------------------------- |
| `false` | `--jsx-bracket-same-line` | `jsxBracketSameLine: <bool>` |
# Arrow Function Parentheses

This comment has been minimized.

@wKovacs64

wKovacs64 Nov 27, 2017

This should probably be a ## heading? 😅

@rattrayalex

rattrayalex approved these changes Nov 27, 2017 edited

LGTM

Thanks for doing this. Disappointed to miss the callbacks option this time around, of course, but I agree it's best to proceed with this for now.

We can wait and see if there's community demand and maybe merge in those changes later.

I don't think there's any lingering doubts about what you have here, so I think it's fine to merge.

I'll be happy to do so within a couple days if I don't hear concerns.

return canPrintParamsWithoutParens(node);
}
return false;

This comment has been minimized.

@rattrayalex

rattrayalex Nov 27, 2017

Collaborator

Probably worth adding a comment indicating that this should be unreachable

This comment has been minimized.

@lipis

lipis Nov 27, 2017

Member

or just

function shouldPrintParamsWithoutParens(path, options) {
  if (options.arrowFunctionParentheses === "avoid") {
    const node = path.getValue();
    return canPrintParamsWithoutParens(node);
  }
  // possible comment here to explain it :)
  return false;
}

This comment has been minimized.

@rattrayalex

rattrayalex Nov 27, 2017

Collaborator

That works too, though I like the explicitness (and having something to grep for, and having a safe default-fallback).

@rattrayalex

This comment has been minimized.

Collaborator

rattrayalex commented Nov 27, 2017

Oops, looks like you have some Travis failures (ironically looks like prettier needs to be run; do we have a command for that?). Also need to fix the docs issue wKovacs64 mentioned.

@azz

This comment has been minimized.

Member

azz commented Nov 27, 2017

Obligatory bikeshed... arrow-function-parentheses is a really long name... Should we call it arrow-parens like ESLint?

@suchipi suchipi added the release:1.9 label Nov 28, 2017

@azz azz added this to the 1.9 milestone Nov 28, 2017

@azz

This comment has been minimized.

Member

azz commented Nov 28, 2017

(Aside: should we continue using milestones, or use release labels?)

@suchipi suchipi removed the release:1.9 label Nov 28, 2017

@suchipi

This comment has been minimized.

Member

suchipi commented Nov 28, 2017

Let's use milestones instead of release labels; I forgot that milestones existed, and they're nicer for this kind of thing.

suchipi added some commits Nov 26, 2017

Add implementation for arrow parens option
Based on #2676

* Thread `path` and `options` through helpers so we don't need to add `needsParens` onto the AST node anymore (mutation)
* Pull test call detection logic out into helper method so it can be re-used for arrow function parens
* Add arrow function parens option implementation (avoid/always)
* Don't break arrow function parens around (done) in test call
Add updated snapshot
Whoops, forgot this

@suchipi suchipi force-pushed the suchipi:arrow_parens branch from 73ac57e to 41a9aba Nov 28, 2017

@suchipi

This comment has been minimized.

Member

suchipi commented Nov 28, 2017

Looks like I'll need to add the option to the website, too.

@duailibe

This comment has been minimized.

Member

duailibe commented Nov 28, 2017

And we should have a way to coordinate pre-release playground options too...

@azz

azz approved these changes Nov 28, 2017

@rattrayalex rattrayalex merged commit 606c5ab into prettier:master Nov 28, 2017

3 of 4 checks passed

codecov/project 96.96% (-0.1%) compared to 172d34e
Details
codecov/patch 86.48% of diff hit (target 80%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
@qleguennec

This comment has been minimized.

qleguennec commented Sep 12, 2018

Any news on adding a block parameter to arrowParens, like the eslint rule?

@rattrayalex

This comment has been minimized.

Collaborator

rattrayalex commented Sep 13, 2018

@qleguennec doubtful; personally, I've been using the "always" option and am much happier with it than I expected. I don't mind the extra parens when the computer does all the typing 😀

Which are you currently using / why is it problematic?

@qleguennec

This comment has been minimized.

qleguennec commented Sep 13, 2018

@rattrayalex My company uses eslint with some default values, including the block parameter, and doesn't use prettier. I just use prettier myself, but can't really use it because of this arrow-paren thingy.

@lydell

This comment has been minimized.

Collaborator

lydell commented Sep 13, 2018

@qleguennec You might be able to work around this using prettier-eslint.

@lock lock bot locked as resolved and limited conversation to collaborators Dec 12, 2018

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