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

Adopting Prettier #3189

Closed
ianobermiller opened this issue Sep 20, 2018 · 6 comments
Closed

Adopting Prettier #3189

ianobermiller opened this issue Sep 20, 2018 · 6 comments

Comments

@ianobermiller
Copy link
Contributor

Would you accept a PR adding prettier to the codebase and formatting all the code? If so, what would you prefer for prettier settings? From the limited code I've seen, it looks like 4 space tabs and single quotes.

@towerofnix
Copy link
Contributor

The ST already uses ESLint for all of their Scratch repositories, which they have tuned to significant detail already - see the repository eslint-config-scratch. I think it would be unlikely for them to use another code formatter.

@benjiwheeler
Copy link
Contributor

@ianobermiller, please let us know if there are particular areas of our formatting that still seem inconsistent or in need of attention!

@rschamp
Copy link
Contributor

rschamp commented Sep 20, 2018

+1 to what both @towerofnix and @benjiwheeler said. We're using eslint and the shared eslint-config-scratch settings across quite a few repos, both public and private.

@rschamp rschamp closed this as completed Sep 20, 2018
@ianobermiller
Copy link
Contributor Author

Prettier is not a replacement for ESLint -- you should still use ESLint of course, as Prettier is only a formatter. The primary reason to use Prettier is that you do not have to think about formatting at all. Once you try it, it releases you from a whole class of things you don't even realize you think about when coding, primarily around fitting code into 120 characters.

This code, for example, from detect-locale.test.js:

Object.defineProperty(window.navigator,
    'language',
    {value: 'pt-BR'}
);

Could just as easily been formatted as:

Object.defineProperty(
    window.navigator,
    'language',
    {value: 'pt-BR'}
);

Or even:

Object.defineProperty(window.navigator, 'language', {value: 'pt-BR'});

Not allowing all three versions of that same code not only makes it easier to write, but also makes the code more consistent to read. Of course there are tradeoffs with it, and you can always use // prettier-ignore comments when you feel strongly that a non-standard formatting is easier to read.

You can see prettier/prettier-eslint#101 (comment) and https://blog.gojekengineering.com/eslint-prettier-for-a-consistent-react-codebase-eaa673debb1d for more examples. Anecdotally, we've been very happy with prettier running on every file in our massive codebase at Facebook, and on my team specifically it has drastically cut down the number of code review comments having to do with formatting.

@benjiwheeler
Copy link
Contributor

benjiwheeler commented Sep 21, 2018

Thanks for the clarification. I'm still curious about how much overlap there is and whether using the two together could be a bit too much of a headache, but I appreciate that there are at least some areas where prettier would add to our consistency.

That said, I've mostly appreciated eslint for capturing and applying conventions around the content of our code, and can't think offhand of times when formatting has been much of an issue.

I'd be curious to see examples of what would change in our codebase. I'm not sure about settings.

@ianobermiller
Copy link
Contributor Author

Thanks for the response, @benjiwheeler!

I'm still curious about how much overlap there is and whether using the two together could be a bit too much of a headache

Prettier and ESLint work very well together: https://prettier.io/docs/en/eslint.html. Basically you don't have to change your existing ESLint workflow at all, it will autofix the formatting through the ESLint standard method. Prettier is just used under the hood, and the plugin takes care of disabling any redundant rules.

can't think offhand of times when formatting has been much of an issue

As someone new to the codebase I can say that consistent formatting would definitely help me to grok unfamiliar code. The example above was one I ran into immediately :)

I'd be curious to see examples of what would change in our codebase. I'm not sure about settings.

I can send out a draft PR, for the most part we can choose settings to match the existing code style.

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

4 participants