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

Low-pri: Use prettier in our repo? #383

Closed
jeffposnick opened this issue Mar 24, 2017 · 12 comments
Closed

Low-pri: Use prettier in our repo? #383

jeffposnick opened this issue Mar 24, 2017 · 12 comments
Labels
Infrastructure Related to the helper and utility scripts in the project.

Comments

@jeffposnick
Copy link
Contributor

Library Affected:
all

Not a top priority, but how would folks feel about running https://github.com/prettier/prettier, configured with our ESLint rules, over the contents of our repo?

It's a good way of standardizing whitespace and other formatting.

@jeffposnick jeffposnick added Infrastructure Related to the helper and utility scripts in the project. question labels Mar 24, 2017
@gauntface
Copy link

I could be convinced either way. At the moment it's annoying enough with I trip up on lint, adding more restrictions could be a pain.

@prateekbh
Copy link
Collaborator

I would love this if a bot can run prettier on every pull request automatically.

@addyosmani addyosmani added the P3 label Mar 31, 2017
@addyosmani
Copy link
Member

Marking as P3. If someone would like to own adding prettier support before stable, let us know and we can assign accordingly.

@jeffposnick
Copy link
Contributor Author

FYI, I came across the following setup while looking at create-react-app's package.json:

{
  "scripts": {
    "format": "prettier --trailing-comma es5 --single-quote --write 'packages/*/*.js' 'packages/*/!(node_modules)/**/*.js'",
    "precommit": "lint-staged"
  },
  "devDependencies": {
    "eslint": "3.16.1",
    "husky": "^0.13.2",
    "lint-staged": "^3.3.1",
    "prettier": "^0.21.0"
  },
  "lint-staged": {
    "*.js": [
      "prettier --trailing-comma es5 --single-quote --write",
      "git add"
    ]
  }
}

I'd have to explore it a bit more, but if we did decide to integrate with prettier, that seems like it might be a model to follow. I think it's based on https://medium.com/@okonetchnikov/make-linting-great-again-f3890e1ad6b8

@prateekbh
Copy link
Collaborator

I am using the same for preact-material-components. Its a pretty quick thing, i'll see if i can plug it in on a separate branch

@KyleAMathews
Copy link

I've been using prettier for Gatsby and other stuff for a while now. Most magical thing ever especially when you set up your editor to run it on save. You can write out super ugly code knowing it'll get prettied up when you save. Really helps ensure a nice consistent codebase.

@gauntface
Copy link

After addy explained that this can make the changes on your behalf, I'm more than willing to give this a trey if someone sets it up.

@jeffposnick
Copy link
Contributor Author

Cool—still low-pri, and it would be good to normalize our ESLint configs (#478) first.

@prateekbh
Copy link
Collaborator

@addyosmani @gauntface i have made changes to bring prettier & lint-staged in the repo. Seems like we have one small hiccup, prettier currently removes parenthesis from arrow function arguments if there is only only argument.

prettier/prettier#812

We might need to make a call if we need to support this in our eslint or drop the plan for prettier. Personally I am pro prettier as this reduces initial contribution nits, and a lot of commenting back and forth.

@gauntface
Copy link

@prateekbh @jeffposnick have either of you managed to get this to run and play nice with out eslint config? If not can we close this issue?

@prateekbh
Copy link
Collaborator

prateekbh commented Jul 18, 2017

Yes, I have a repo where prettier is integrated, but it conflicts with our lint configuration.
Prettier says upfront that it is "opinionated formatter", though it has flags to configure some stuff but not all.

e.g.
--single-quotes flag turn everything into single quotes except a code like

const quote = 'lorem ipsum\'s'

this gets converted to

const quote = "lorem ipsum's"

also it removes braces around arrow functions if there's only one parameter

 event => {
   ...
 }

If you guys think we can make an exception for this in our eslint config, then I can go ahead and submit the PR, else feel free to close the issue

@jeffposnick
Copy link
Contributor Author

Oh, that's a bummer about prettier. I thought it would read in our ESLint config and adhere to those rules, but looking into it more, I guess that folks who use both end up relaxing/changing their ESLint config to use prettier, not the other way around.

The one-time eslint --fix that I did in the previous PR managed to resolve some of the outstanding conflicts with the latest rules, and I'm happy enough keeping our up-to-date ESLint config in place without everything being automated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Infrastructure Related to the helper and utility scripts in the project.
Projects
None yet
Development

No branches or pull requests

5 participants