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

Try running prettier on the codebase #23

Closed
wants to merge 1 commit into from
Closed

Conversation

randycoulman
Copy link
Contributor

@randycoulman randycoulman commented Jan 21, 2017

DO NOT MERGE

This is a spike to see what impact running prettier on our code would have.

If this works, the idea would be that we would disable any ESLint formatting rules and replace them with the use of automatic formatting by prettier.

I ran prettier on each JavaScript file as prettier —single-quote —write file.js.

Based on these results (and running it on a client's React Native codebase), I have the following observations so far:

  • We’d have to be OK with some changes to our current formatting. IMO, if the community adopts prettier’s opinionated formatting, then this is an acceptable tradeoff. I’m willing to give up being a special snowflake in the name of community consistency.

  • Prettier adds trailing semi-colons with no option to disable them. If we use an editor auto-fix integration, we won’t have to type them ourselves at least.

  • Prettier has an option to add or not add spaces inside array brackets and curly braces. Our current style is to add them for curly braces but not brackets. One of the things we’d have to live with, but there is an open issue to consider splitting these two.

  • Promise chains get formatted like pipelines. Not sure what I think about this, but can probably live with it (see app/index.js for an example).

  • It incorrectly formats code using the Babel export-extensions-transform by adding incorrect curly braces. I’ve opened an issue for this.

  • Strings in JSX are always written with double-quotes regardless of the command-line option. This is apparently intentional. See General JSX Improvements prettier/prettier#73 (comment) and General JSX Improvements prettier/prettier#73 (comment) for some of the reasoning.

  • It deletes comments in some cases. See app/templates/client/modules/app/reducer/index.js for an example.

  • It likes to squash multi-line object literals down to one line if they'll fit. This makes some code hard to read, especially in react-native stylesheet files.

  • It likes to squash Ramda compose pipelines down to one line if they'll fit. This also makes some code hard to read.

  • It formats multi-line React components differently; I'm ok with this:

Ours:

<Component
    prop={value} />

Prettier:

<Component
  prop={value}
/>

Conclusion: Promising, but not yet ready for prime time. Worth keeping an eye on it and trying again in a month or two.

@lexun
Copy link
Contributor

lexun commented Feb 4, 2017

I've been using it in personal projects and in spite of the issues I've found the gains to be well worth the tradeoff. The efficiency gained and time saved is more than I expected, especially when refactoring things and using the editor package. I'm also ok with things like semicolons everywhere if I don't have to write them :)

If we adopt though, I'd vote we get on board with all the defaults including double quotes. It'd be nice to install the editor package and be able to use it in all projects without having to adjust settings just for zeal projects. Assuming it doesn't respect a config file in your project that is. Either way if this thing gains traction, it could lead to a lot of community wide uniformity, and going with the flow might be best.

@randycoulman
Copy link
Contributor Author

I ran another test with the latest version of prettier, and most of the issues I outlined above have been addressed. I didn't create a new PR with the changes, but I can if desired.

At this point, I'd be in favor of adopting prettier for all new projects.

Here's a proposed approach:

  1. Modify eslint-config-zeal to copy all formatting-related rules to a separate file (no-prettier or something like that) and then turn all of those rules to off in the default configuration. That way, we assume prettier by default, but for projects that don't use it, they can extend zeal/no-prettier to get back our original formatting rules.

  2. Consider modifying the rules in no-prettier to match the output produced by prettier to make it easier to migrate in the future.

  3. Re-do this PR with the latest version of prettier and the latest code on master, then merge it in.

  4. Figure out how we want to enforce prettier formatting. Do we rely on everyone to use their editor to do it? Set up a precommit/prepublish hook to do it for us, maybe using lint-staged as documented in prettier's README? Other ideas?

@lexun
Copy link
Contributor

lexun commented Feb 17, 2017

Sounds great to me. I'm not sure I like the idea of using git hooks / lint-staged though. I'd rather just expect that people will have it set up with their editors and then use things like eslint-plugin-prettier with eslint-config-prettier. Then if the formatting is wrong for whatever reason CI would complain as usual.

@randycoulman
Copy link
Contributor Author

@lexun Those look like good tools to adopt. I'd forgotten they existed.

@randycoulman
Copy link
Contributor Author

Closing; prettier has evolved a lot since this spike. We can create a new PR with the latest version when we're ready to do so.

@randycoulman randycoulman deleted the spikes/prettier branch May 16, 2017 16:24
@lexun lexun mentioned this pull request May 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants