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.

Sign up for GitHub

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

Jump to bottom

Add Prettier (Fixes #22919) #24323

Closed
wants to merge 3 commits into from
Closed

Add Prettier (Fixes #22919) #24323

wants to merge 3 commits into from

Conversation

lipis
Copy link
Contributor

@lipis lipis commented Oct 10, 2017 •
edited

Fixes #22919

I removed bunch of styling rules.. so feel free to comment if we can remove more stylistic rules since most of them are covered by Prettier.

/cc @XhmikosR

@lipis lipis mentioned this pull request Oct 10, 2017
Closed
lipis
lipis commented Oct 10, 2017
View reviewed changes
package.json
@@ -33,6 +33,7 @@
"js": "npm-run-all js-lint* js-compile* js-minify*",
"js-main": "npm-run-all js-lint js-compile js-minify",
"js-docs": "npm-run-all js-lint-docs js-minify-docs",
"js-lint-fix": "eslint --fix js/ && eslint --fix --config js/tests/.eslintrc.json --env node build/",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm open for suggestions on how to name the command and if we can do some optimisations here.

@XhmikosR
Copy link
Member

XhmikosR commented Oct 10, 2017 •
edited

First of all, thanks for the PR!

There are many things I personally don't like in this config. I think we are better off just tweaking our rules.

  1. For example, removing the spaces around = and : in objects are OK with me, but I know Johann is using them.

  2. We could look into the chaining methods breaking but this is too much IMO:

    fs.createReadStream(workboxSWSrcPath).pipe(fs.createWriteStream(workboxSWDestPath))
-fs.createReadStream(workboxSWSrcMapPath).pipe(fs.createWriteStream(workboxSWDestMapPath))
+fs
+  .createReadStream(workboxSWSrcPath)
+  .pipe(fs.createWriteStream(workboxSWDestPath))
+fs
+  .createReadStream(workboxSWSrcMapPath)
+  .pipe(fs.createWriteStream(workboxSWDestMapPath))

  3. Having the brackets in the same line is definitely something I'd like to have.

  4. Removing the spaces before object : is definitely something I'd like to have.

  5. Removing the space before a function's parentheses is something I personally don't like.

  6. This kind of changes are also OK to me:

    -      const rootElement      = $(this._element).closest(
-        Selector.DATA_TOGGLE
-      )[0]
+      const rootElement = $(this._element).closest(Selector.DATA_TOGGLE)[0]

  7. arrow-parens I think I find it better to keep them all the time

  8. The line width is just too much; screens are too big nowadays to have a limit of 80. Personally I'd try using 120, but I think we better have this off anyway.

That being said, I still think we should first tweak our ESLint rules. And perhaps change the location of .eslintrc.json.

/CC @Johann-S for feedback

houndci-bot
houndci-bot reviewed Oct 10, 2017
View reviewed changes
dist/css/bootstrap.css Outdated
}

.font-weight-bold {
font-weight: bold !important;
font-weight: 700 !important;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unexpected !important declaration-no-important

dist/css/bootstrap.css Outdated
@@ -8232,11 +8258,11 @@ a.bg-dark:focus, a.bg-dark:hover {
}

.font-weight-normal {
font-weight: normal !important;
font-weight: 400 !important;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unexpected !important declaration-no-important

dist/css/bootstrap.css Outdated
@@ -4194,16 +4220,16 @@ tbody.collapse.show {
.card-group .card:last-child .card-img-bottom {
border-bottom-left-radius: 0;
}
.card-group .card:only-child {
border-radius: 0.25rem;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unexpected leading zero number-leading-zero

dist/css/bootstrap.css Outdated

.card > .list-group:last-child .list-group-item:last-child {
border-bottom-right-radius: 0.25rem;
border-bottom-left-radius: 0.25rem;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unexpected leading zero number-leading-zero

dist/css/bootstrap.css Outdated
}

.card > .list-group:last-child .list-group-item:last-child {
border-bottom-right-radius: 0.25rem;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unexpected leading zero number-leading-zero

dist/css/bootstrap.css Outdated
@@ -2753,15 +2758,15 @@ fieldset[disabled] a.btn {
background-color: transparent;
}

.btn-outline-dark:active, .btn-outline-dark.active,
.btn-outline-dark:not([disabled]):not(.disabled):active, .btn-outline-dark:not([disabled]):not(.disabled).active,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expected newline after "," selector-list-comma-newline-after

dist/css/bootstrap.css Outdated
@@ -2724,7 +2729,7 @@ fieldset[disabled] a.btn {
background-color: transparent;
}

.btn-outline-light:active, .btn-outline-light.active,
.btn-outline-light:not([disabled]):not(.disabled):active, .btn-outline-light:not([disabled]):not(.disabled).active,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expected newline after "," selector-list-comma-newline-after

dist/css/bootstrap.css Outdated
@@ -2695,7 +2700,7 @@ fieldset[disabled] a.btn {
background-color: transparent;
}

.btn-outline-danger:active, .btn-outline-danger.active,
.btn-outline-danger:not([disabled]):not(.disabled):active, .btn-outline-danger:not([disabled]):not(.disabled).active,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expected newline after "," selector-list-comma-newline-after

dist/css/bootstrap.css Outdated
@@ -2666,7 +2671,7 @@ fieldset[disabled] a.btn {
background-color: transparent;
}

.btn-outline-warning:active, .btn-outline-warning.active,
.btn-outline-warning:not([disabled]):not(.disabled):active, .btn-outline-warning:not([disabled]):not(.disabled).active,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expected newline after "," selector-list-comma-newline-after

dist/css/bootstrap.css Outdated
@@ -2637,7 +2642,7 @@ fieldset[disabled] a.btn {
background-color: transparent;
}

.btn-outline-info:active, .btn-outline-info.active,
.btn-outline-info:not([disabled]):not(.disabled):active, .btn-outline-info:not([disabled]):not(.disabled).active,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expected newline after "," selector-list-comma-newline-after

@XhmikosR
Copy link
Member

Also, don't commit dist files. Although Hound should ignore those, I'll have a look at this.

@XhmikosR XhmikosR mentioned this pull request Oct 10, 2017
Merged
@lipis
Copy link
Contributor Author

lipis commented Oct 10, 2017

As you might know already.. Prettier doesn't have many options and that's why we have to ignore/comply with how Prettier is doing it (point 5 and 7 - not much we can do here).

  1. I increased the size to 120.. waiting for more comments.

@XhmikosR
Copy link
Member

That is why the points I made above are mostly so that we tweak our existent ESLint config and drop the whole idea of prettier.

@FezVrasta
Copy link
Contributor

You may use prettier-eslint to get the best of both worlds

@FezVrasta
Copy link
Contributor

Also, I think it would help the review process if you splitted the PR in two commits: 1 about the Prettier integration, 1 about the Prettier formatting of the files.

@lipis
Copy link
Contributor Author

lipis commented Oct 10, 2017

I'll do that.. with the commits, because at the moment I'm using the the eslint and Prettier..

@XhmikosR
Copy link
Member

I think we are better off with using https://standardjs.com/ even though it's not a formatter.

Personally I don't like the changes prettier makes here judging from the files committed.

/CC @Johann-S

@lipis
Copy link
Contributor Author

lipis commented Oct 10, 2017 •
edited

Of course it's an opinionated code formatter and if there are some things that we don't like there is not much we can do, as earlier stated.

Having said that I have two commits in this PR now to separate the config from formatting.

@FezVrasta
Copy link
Contributor

FezVrasta commented Oct 10, 2017 •
edited

@XhmikosR try to look beyond the formatting changes, think that you will never have to think about how your code looks like and you will never have to manually format a line of code in your life.

That's liberating, and boosts productivity.

Apart from that, all the contributions will already follow the "convention" because Prettier will take care of it for you guys.

On Popper.js I use it because I don't give a **** about how my code is formatted as long it's consistent and readable, Prettier gives me exactly this.

https://medium.freecodecamp.org/why-robots-should-format-our-code-159fd06d17f7

@XhmikosR
Copy link
Member

Like I said, I don't like the style it's enforcing. Like, a lot. It's a matter of personal taste.

That being said, wait for Johann to chime in too.

@Johann-S
Copy link
Member

Sorry for the waiting 😄

I understand how it's convenient to not have to think how to format your code, that being said as @XhmikosR they are few rules I don't like...

  • The chaining methods breaking (because it's too much for me here)
  • I prefer having the brackets in the same line.
  • Removing the spaces before object, same I'd like to have that.
  • Removing the space before a function's parentheses
  • arrow-parens I prefer keep them all the time
  • 120 in terms of line width is fine for me too

But that's a matter of preferences, here finally I'm not sure about adding prettier to our workflow or standard js too.
I would prefer to tweak our Eslint rules and add a pre-commit hook which will format our code (eslint --fix)

@FezVrasta
Copy link
Contributor

@Johann-S all your points can be covered by some custom eslint rule ran after Prettier.

Except for the chaining methods, because they don't just break on multi line always, but only when they are too long to live in a single line.

ESlint can't cover all the cases covered by Prettier, on the other hand.

@lipis
Copy link
Contributor Author

lipis commented Oct 11, 2017 •
edited

Plus Bootstrap will be featured here as well.. as the most starred library using Prettier :) hehe

https://prettier.io

@lipis
Copy link
Contributor Author

lipis commented Oct 11, 2017

Channing methods it is not the best solution, because it will be hard also for the editor to figure out what's the correct way to format things...! We either go full Prettier or not at all.. and rely on other things for the formatting.

@bennycode
Copy link

ESlint can't cover all the cases covered by Prettier, on the other hand.

@FezVrasta What are the cases (for JS & JSX code formatting) that ESLint can not handle?

@FezVrasta
Copy link
Contributor

prettier/prettier-eslint#101

@bennycode
Copy link

prettier/prettier-eslint#101

The only precise example in that issue is doing auto formatting for code that exceeds the maximum limit of characters per line. That's something modern IDEs have already built in.

I am also not convinced by the "Gives you an integrated experience" (prettier/prettier-eslint#101 (comment)) claim. I would argue that the integrated experience of ESLint in WebStorm is much more mature.

@FezVrasta
Copy link
Contributor

Yeah I mean, I'm not paid to convince you, whatever 😅

@lipis
Copy link
Contributor Author

lipis commented Oct 11, 2017

WebStorm costs some 💶 💵

@bennycode
Copy link

@FezVrasta No offense! I don't want you to be a sales person. 😀

I am just looking for hard facts what makes Prettier better than ESLint (for JS & JSX code formatting) and thought that I can maybe find the answers here. 😃

This was referenced Oct 11, 2017
Closed
Merged
@Johann-S
Copy link
Member

Thank you all for your feedbacks and @lipis for your PR but we will pass on adding prettier to our workflow.

But we will tweak our Eslint rules and a git hook which will run eslint --fix

@Johann-S Johann-S closed this Oct 11, 2017
@Johann-S Johann-S mentioned this pull request Oct 11, 2017
Closed
@lipis
Copy link
Contributor Author

lipis commented Oct 11, 2017

Fair enough.. see you in a few months.. :)

@lipis lipis deleted the prettier branch October 25, 2017 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@houndci-bot houndci-bot houndci-bot left review comments

Assignees
No one assigned
Labels
js meta v4
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@lipis @XhmikosR @FezVrasta @Johann-S @bennycode @houndci-bot