Fixes #1434 - adding Prettier #1468

magsout · 2017-03-31T19:16:15Z

r? @miketaylr you write all JS of webcompat.

Every commit, this recipe https://github.com/webcompat/webcompat.com/pull/1468/files#diff-b9cfc7f2cdf78a7f4b91a753d10865a2R56 Is triggered.

if any JS is modified, it is automatically added after a git commit https://github.com/webcompat/webcompat.com/pull/1468/files#diff-b9cfc7f2cdf78a7f4b91a753d10865a2R62 ? I'm not sure if it's a good idea.

The configuration is simple, I have modified only this option --trailing-com. (added a comma at the end of object)

miketaylr · 2017-03-31T20:55:43Z

Would it be possible to split out the commit with all the changes to the code? Ideally there would be one commit for removing eslint, one for adding prettier, and one for transforming code.

Also, we'll need to update the .travis.yml config... right now it's exploding on npm run lint.

miketaylr · 2017-03-31T20:57:18Z

I'd be interested to have @denschub take a look at this PR as well.

miketaylr · 2017-03-31T21:03:41Z

There's still some eslint specific comments in the code, for example:

webcompat.com/webcompat/static/js/lib/comments.js

Line 5 in ea87d2f

var issues = issues || {}; // eslint-disable-line no-use-before-define

So if we decide there's no more usage for eslint, we should remove those.

I wonder if we should document how this affects workflow in CONTRIBUTING.md as well.

miketaylr · 2017-03-31T21:06:51Z

I wonder how people are using this w/ eslint though... with this change we lose out on all the recommended linting rules: http://eslint.org/docs/rules/ (and the few that we have added).

magsout · 2017-03-31T21:06:58Z

@miketaylr hum yes, I may have been a bit quick on this PR.

tests/functional/comments-auth.js

-  });
-});
+      "Add a screenshot to a comment": function() {
+        return FunctionalHelpers.openPage(


magsout · 2017-03-31T21:11:31Z

I wonder how people are using this w/ eslint though... with this change we lose out on all the recommended linting rules: http://eslint.org/docs/rules/ (and the few that we have added).

@miketaylr Yes, it's Prettier's role, but maybe it's a bit too gross? You find that you lose what compared to eslint?

miketaylr · 2017-03-31T21:16:41Z

Yes, it's Prettier's role, but maybe it's a bit too gross? You find that you lose what compared to eslint?

We lose the "safety" stuff that eslint was checking. eslint was also checking boring things like, do you have a space before a paren -- in my mind, this is what Prettier is good at. Don't think about style, let the machine do style. But it doesn't tell us if we accidentally introduced a new global variable, for example: https://github.com/webcompat/webcompat.com/blob/master/.eslintrc#L6

It should be possible to have both eslint and prettier -- just get rid of the code style rules from eslint, and run prettier after that?

I didn't study these yet, but I see:

https://github.com/prettier/eslint-config-prettier
https://github.com/prettier/prettier-eslint

magsout · 2017-03-31T21:26:21Z

But it doesn't tell us if we accidentally introduced a new global variable, for example: https://github.com/webcompat/webcompat.com/blob/master/.eslintrc#L6

@miketaylr right now, this recipe https://github.com/webcompat/webcompat.com/blob/master/package.json#L48 handle your comment above ?

magsout · 2017-03-31T21:31:16Z

@miketaylr

hum, let me rethink completely my PR.
Thanks for your feedback and your review.

miketaylr · 2017-03-31T21:36:51Z

@miketaylr right now, this recipe https://github.com/webcompat/webcompat.com/blob/master/package.json#L48 handle your comment above ?

Yeah, I guess. (But I never use that...)

To be clear, I'm +1 on prettier, it does way more than eslint --fix. I just don't want us to lose linting non-style stuff.

magsout · 2017-03-31T23:14:23Z

To be clear, I'm +1 on prettier, it does way more than eslint --fix. I just don't want us to lose linting non-style stuff.

Yeah, and I agree with you.

magsout · 2017-04-01T15:49:00Z

@miketaylr another approch. Combined prettier with eslint. I have removed rules of eslint and have added prettier instead. We keep our lint/fix, with the Formatted Code by Prettier.

What do you think ?

Gruntfile.js

@@ -9,7 +9,7 @@ module.exports = function(grunt) {
    cssPath: "webcompat/static/css",
    imgPath: "webcompat/static/img",
    banner: "/*! <%= pkg.title %>\n" +
-      " *  Copyright (c) <%= grunt.template.today(\"yyyy\") %> <%= pkg.author.name %>\n" +
+      ' *  Copyright (c) <%= grunt.template.today("yyyy") %> <%= pkg.author.name %>\n' +


miketaylr · 2017-04-03T19:48:08Z

Interesting, while trying this locally I got the following error:

🐓 npm run lint

> webcompat@ lint /Users/miket/dev/compat/webcompat.com
> eslint ./Gruntfile.js ./tests ./grunt-tasks ./webcompat/static/js/lib


/Users/miket/dev/compat/webcompat.com/webcompat/static/js/lib/issues.js
  118:19  error  Follow `prettier` formatting (expected '\n' but found '"')  prettier/prettier

✖ 1 problem (1 error, 0 warnings)

Wonder why that didn't get picked up... (I can fix it up).

miketaylr · 2017-04-03T19:51:13Z

Interesting, while trying this locally I got the following error:

(ah that's because that code got merged in before @magsout made this PR. forgive me, I'm v slow sometimes...)

miketaylr · 2017-04-03T21:13:46Z

@magsout this looks good, we should probably update CONTRIBUTING.md as well. One question about workflow... for some imaginary scenario:

write some code
run npm run lint
run npm run fix (if there were simple style errors...)
commit, make PR, etc.

Does that seem like the best way?

miketaylr · 2017-04-03T21:18:48Z

Actually, I think we can merge this and I'll file a follow up issue to document recommended workflow.

magsout · 2017-04-03T22:04:40Z

@miketaylr yes, it does 👍

magsout added lang: JS status: needs review labels Mar 31, 2017

magsout requested a review from miketaylr March 31, 2017 19:16

magsout added the in progress label Mar 31, 2017

miketaylr changed the title ~~Issue #1434 - adding Prettier~~ Fixes #1434 - adding Prettier Mar 31, 2017

miketaylr reviewed Mar 31, 2017

View reviewed changes

tests/functional/comments-auth.js

});

});

"Add a screenshot to a comment": function() {

return FunctionalHelpers.openPage(

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

magsout force-pushed the issues/1434 branch from ea87d2f to 71a57f6 Compare April 1, 2017 15:47

magsout added the type: testing label Apr 1, 2017

miketaylr reviewed Apr 3, 2017

View reviewed changes

magsout added 2 commits April 3, 2017 14:54

Issue #1434 - added Prettier and removed rues from eslint

ba76392

Issue #1434 - run fix with prettier

94fd435

miketaylr force-pushed the issues/1434 branch from 80c97c3 to 94fd435 Compare April 3, 2017 19:55

miketaylr merged commit 30cfdba into master Apr 3, 2017

miketaylr removed the in progress label Apr 3, 2017

miketaylr mentioned this pull request Apr 3, 2017

Document recommended eslint/prettier workflow #1474

Closed

miketaylr deleted the issues/1434 branch April 3, 2017 21:24

miketaylr added the status: add to changelog label Apr 3, 2017

magsout mentioned this pull request Apr 4, 2017

Add prettier instead of eslint #1434

Closed

miketaylr removed the status: add to changelog label Apr 7, 2017

snyk-bot mentioned this pull request Sep 16, 2021

[Snyk] Security upgrade yargs from 13.3.2 to 15.0.1 sirinartk/webcompat.com#14

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #1434 - adding Prettier #1468

Fixes #1434 - adding Prettier #1468

magsout commented Mar 31, 2017 •

edited by miketaylr

Loading

miketaylr commented Mar 31, 2017

miketaylr commented Mar 31, 2017

miketaylr commented Mar 31, 2017

miketaylr commented Mar 31, 2017

magsout commented Mar 31, 2017

This comment was marked as abuse.

This comment was marked as abuse.

magsout commented Mar 31, 2017

miketaylr commented Mar 31, 2017

magsout commented Mar 31, 2017

magsout commented Mar 31, 2017

miketaylr commented Mar 31, 2017

magsout commented Mar 31, 2017

magsout commented Apr 1, 2017 •

edited

Loading

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

miketaylr commented Apr 3, 2017

miketaylr commented Apr 3, 2017

miketaylr commented Apr 3, 2017

miketaylr commented Apr 3, 2017

magsout commented Apr 3, 2017

Fixes #1434 - adding Prettier #1468

Fixes #1434 - adding Prettier #1468

Conversation

magsout commented Mar 31, 2017 • edited by miketaylr Loading

miketaylr commented Mar 31, 2017

miketaylr commented Mar 31, 2017

miketaylr commented Mar 31, 2017

miketaylr commented Mar 31, 2017

magsout commented Mar 31, 2017

This comment was marked as abuse.

This comment was marked as abuse.

magsout commented Mar 31, 2017

miketaylr commented Mar 31, 2017

magsout commented Mar 31, 2017

magsout commented Mar 31, 2017

miketaylr commented Mar 31, 2017

magsout commented Mar 31, 2017

magsout commented Apr 1, 2017 • edited Loading

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

miketaylr commented Apr 3, 2017

miketaylr commented Apr 3, 2017

miketaylr commented Apr 3, 2017

miketaylr commented Apr 3, 2017

magsout commented Apr 3, 2017

magsout commented Mar 31, 2017 •

edited by miketaylr

Loading

magsout commented Apr 1, 2017 •

edited

Loading