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

Consider adopting prettier #262

Closed
davidcornu opened this issue Jun 30, 2017 · 9 comments
Closed

Consider adopting prettier #262

davidcornu opened this issue Jun 30, 2017 · 9 comments

Comments

@davidcornu
Copy link
Contributor

davidcornu commented Jun 30, 2017

https://github.com/prettier/prettier has a lot more details.

It's been adopted by quite a few projects as the de facto style guide (ie. run your code through it and call it a day - similar to gofmt).

With a few configuration options

prettier --trailing-comma=es5 --single-quote --no-bracket-spacing

the output comes pretty close to our ESLint rules.

The biggest offender I've found so far is we prefer

(arg) => { /* ... */ }

over

arg => { /* ... */ }

for lambdas that take a single argument.

@lemonmade
Copy link
Member

Yeah, I would love to recommend this, I'm willing to relent on the single param functions (even though it's a terrible choice). We had been waiting on that rule, and on full TypeScript support, before jumping on it, and now that TS is in it seems reasonable. What do you think we'd need to do here to make this a clear recommendation? Instructions on how to use it with the linter? Recommended options? An examples use of it in core?

@davidcornu
Copy link
Contributor Author

What do you think we'd need to do here to make this a clear recommendation? Instructions on how to use it with the linter? Recommended options? An examples use of it in core?

As far as I can tell, most projects that have adopted it go through a process along the lines of

  1. Update ESLint rules to be compatible with Prettier
  2. Add a yarn run prettier that formats everything with the preferred options
  3. Convert the entire code base in one fell swoop
  4. Implement something like Failing to run prettier should block CI facebook/react#9187 using prettier --list-different on CI to prevent any un-prettier-ed code from being committed

@davidcornu
Copy link
Contributor Author

^ once that's done people can update their editor configs or pre-commit hooks to suit their workflows.

@kaelig
Copy link

kaelig commented Jul 17, 2017

As discussed with @lemonmade over Slack, I'd like to do a spike on this and see how far I go towards seamless integration of prettier in our current process (on Shopify/polaris-react). As someone who's just joined I feel like this could be super useful to other new developers so worth investigating. I'll post the findings here, and from there we can decide of actions for other projects.

cc @dfmcphee

@clauderic
Copy link
Member

+1 for this, I think this would be a great leap towards a more consistent codebase, especially if implemented at the CI level and as a pre-commit hook.

As far as pre-commit hooks go, a lot of large-scale projects nowadays are using a combination of husky and lint-staged to make them easily shareable across the team without needing to be manually installed and to make sure to only run against staged files so they run faster. You can check out the babel package.json for an example of how that works in practice.

@kaelig
Copy link

kaelig commented Jul 17, 2017

Thanks for the pointers. I was initially thinking we could encourage developers to install a Prettier extension in their text editor, but I guess pre-commit hooks scale much better!

I haven't had experience with modifying code at the CI level. Would that be something developers are comfortable with?

@GoodForOneFare
Copy link
Member

Please check with #dev-accel before adding prehooks. Last time I discussed that, they had a strong preference for finding alternatives.

@kaelig
Copy link

kaelig commented Jul 18, 2017

That's good to know! I'll make sure to have this in mind and will chat in case hooks seem like the best solution.

@kaelig
Copy link

kaelig commented Oct 1, 2018

Closing as we are now leveraging prettier!

@kaelig kaelig closed this as completed Oct 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants