Add opt-in prettier #15106

mattapperson · 2017-11-22T02:11:25Z

I added prettier in this PR in line with the discussion in #14810.

Maybe this won't be accepted and that is OK, I just wanted to have something "real" to discuss and accept/alter/reject vs speaking in abstract

Notes:

This is opt-in, the .prettierignore file lists all main kibana directories and then each plugin separately. This way each plugin can be moved one at a time to using prettier.
I used a lint-staged plugin to ensure that going forward, any prettier opted-in plugins/sections of kibana stay formatted to prettier without getting in the way of those that are not.
Once you remove a line from the .prettierignore file, you will want to add it to the eslitignore file to prevent the old system and eslint rules from preventing you from committing

kimjoar

Awesome, thanks for creating this PR, @mattapperson.

I like this setup with the ignore file. I wish Prettier could run multiple ignore files at the same time, then we could just have it run for both .gitignore and for .prettierignore at it would simplify this file a bit, but looks like it only supports a single file: https://prettier.io/docs/en/cli.html#ignore-path

I also added a note about the current precommit setup in Kibana, so we don't end up with two setups.

I definitely think this will help us gradually move towards Prettier 🎉 👏

kimjoar · 2017-11-22T12:46:40Z

package.json

@@ -54,6 +51,7 @@
    "build": "grunt build",
    "release": "grunt release",
    "start": "sh ./bin/kibana --dev",
+    "prettier": "./node_modules/prettier/bin/prettier.js \"./**/*.js\" --write",


When you have prettier in the deps, npm already makes the bin available, so this could just be:

"prettier": "prettier --write './**/*.js'"

Correct me if I’m wrong, but if someone has another version installed globally, won’t it use the global version first if you do that?

It will use the local first, then the global if no local exists.

Gtk thanks!

kimjoar · 2017-11-22T12:48:32Z

package.json

@@ -21,6 +14,10 @@
  "bugs": {
    "url": "http://github.com/elastic/kibana/issues"
  },
+  "pre-commit": ["pre-commit"],


Kibana already has a precommit setup that's triggered a bit further down in this file:

kibana/package.json

Line 57 in b31f736

"precommit": "node scripts/precommit_hook",

It ends up running this file: https://github.com/elastic/kibana/blob/master/src/dev/run_precommit_hook.js

I think we should probably add this prettier setup there so we don't have multiple precommit setups (That might mean that we should create a scripts file for running Prettier instead, see https://github.com/elastic/kibana/tree/master/scripts). Let me know if you want to zoom or something about this setup. We've also gradually moved more towards having scripts in the ./scripts folder (though not sure it's needed here)

cc @spalger, who knows all about this setup

This I did intentionally so as we finish the migration the mass of stuff needed for eslint could be removed in favor of something simpler and also the generally accepted community recommended setup for prettier. However if this is not desired here I’m happy to change that attack plan. @spalger ?

kimjoar · 2017-11-22T12:51:00Z

package.json

@@ -215,6 +214,8 @@
    "yauzl": "2.7.0"
  },
  "devDependencies": {
+    "lint-staged": "5.0.0",
+    "prettier": "1.8.2",


Btw, if you use npm install --save-dev prettier the dep will be added to the "correct" location in devDependencies (when npm install X or yarn add X runs on the file, it will sort the deps alphabetically, so if we add a dep to the top here we'll likely have to re-commit this later when npm or yarn has moved them around)

Bah! I thought yarn did this even if you pasted in once you installed it. NPM used to right? Idk oh well yea that needs to be correctly sorted.

kimjoar · 2017-11-22T12:51:12Z

package.json

-  ],
+  "description":
+    "Kibana is an open source (Apache Licensed), browser based analytics and search dashboard for Elasticsearch. Kibana is a snap to setup and start using. Kibana strives to be easy to get started with, while also being flexible and powerful, just like Elasticsearch.",
+  "keywords": ["kibana", "elasticsearch", "logstash", "analytics", "visualizations", "dashboards", "dashboarding"],


nitpick: revert these changes ^?

heh yea sorry, this was prettier running on the file before I added it to the ignore list lol

kimjoar · 2017-11-22T12:52:52Z

.prettierrc

@@ -0,0 +1,4 @@
+{
+  "singleQuote": true,
+  "semi": true


Maybe just go with the defaults here? (#14810 (comment))

However, I'm +1 on having the .prettierrc though, even if it just becomes

{ }

for now

Ah cool, yea I missed that comment.

spalger · 2017-11-27T16:49:31Z

package.json

@@ -21,6 +21,15 @@
  "bugs": {
    "url": "http://github.com/elastic/kibana/issues"
  },
+  "pre-commit": [


What is this config for? We already use husky to call npm run precommit, do we need both of these?

spalger · 2017-11-27T16:51:03Z

package.json

@@ -53,6 +62,7 @@
    "checkLicenses": "grunt licenses",
    "build": "grunt build",
    "release": "grunt release",
+    "prettier": "prettier \"./**/*.js\" --write",


I'd prefer that this was node scripts/prettier rather than an npm script, See #11095

mattapperson · 2017-11-27T21:37:56Z

@spalger updated with your assistance :)

spalger · 2017-11-27T21:48:35Z

src/dev/run_prettier.js

+  process.argv.push('./**/*.js');
+}
+
+if (!flags.write) {


Does this allow --no-write to work here? Might have to check for the flags.noWrite, or === false

@spalger I don't see the need to support that. This command is just to let a team convert their entire project in one PR vs how editor plugins work converting 1 file at a time.

kimjoar

I had a couple nitpicks, but other than those I think we should get this is in.

Prettier 1.9.0 just got released, so would be great to update that dep first.

kimjoar · 2017-12-05T09:44:23Z

package.json

+  "lint-staged": {
+    "*.js": [
+      "prettier --list-different",
+      "git add"


Can we remove this git add? I don't like tooling doing git stuff for me.

kimjoar · 2017-12-05T09:44:51Z

package.json

@@ -67,6 +74,7 @@
    "uiFramework:createComponent": "yo ./ui_framework/generator-kui/app/component.js",
    "uiFramework:documentComponent": "yo ./ui_framework/generator-kui/app/documentation.js"
  },
+


remove this line

spalger · 2017-12-28T19:10:02Z

I've been using prettier eslint plugin with much success lately. Do you think we could just go that route so we're adding a new tool but rather extending a tool we're already using? If we do then we could just add the prettier/prettier rule to a subset of files via eslint overrides.

kimjoar · 2018-02-05T18:33:05Z

Closed by #16514

Matt Apperson added 2 commits November 21, 2017 21:01

Add opt-in prettier

a4eaf9f

ignore some files wherever they are

0f9ffb0

mattapperson requested review from kimjoar, epixa and simianhacker November 22, 2017 02:12

kimjoar reviewed Nov 22, 2017

View reviewed changes

use std settings for prettier and cleaned up package.json

8fbb538

mattapperson requested a review from spalger November 27, 2017 16:46

Merge branch 'master' into add-prettier

a3938b7

spalger suggested changes Nov 27, 2017

View reviewed changes

Matt Apperson and others added 3 commits November 27, 2017 13:02

cleanup package.json

13d64de

update docs to mention prettier and prettier setup

95790f2

Added prettier as a node script vs npm script

b093d13

spalger reviewed Nov 27, 2017

View reviewed changes

kimjoar approved these changes Dec 5, 2017

View reviewed changes

kimjoar mentioned this pull request Jan 28, 2018

Run Prettier for JS files #16338

Closed

kimjoar closed this Feb 5, 2018

mattapperson deleted the add-prettier branch August 7, 2018 13:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add opt-in prettier #15106

Add opt-in prettier #15106

mattapperson commented Nov 22, 2017

kimjoar left a comment

kimjoar Nov 22, 2017

mattapperson Nov 22, 2017

kimjoar Nov 22, 2017

mattapperson Nov 22, 2017

kimjoar Nov 22, 2017

mattapperson Nov 22, 2017

kimjoar Nov 22, 2017

mattapperson Nov 22, 2017

kimjoar Nov 22, 2017

mattapperson Nov 22, 2017

kimjoar Nov 22, 2017

mattapperson Nov 22, 2017

spalger Nov 27, 2017

spalger Nov 27, 2017

mattapperson commented Nov 27, 2017

spalger Nov 27, 2017

mattapperson Nov 28, 2017

kimjoar left a comment

kimjoar Dec 5, 2017 •

edited

Loading

kimjoar Dec 5, 2017

spalger commented Dec 28, 2017 •

edited

Loading

kimjoar commented Feb 5, 2018

Add opt-in prettier #15106

Add opt-in prettier #15106

Conversation

mattapperson commented Nov 22, 2017

kimjoar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattapperson commented Nov 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kimjoar left a comment

Choose a reason for hiding this comment

kimjoar Dec 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalger commented Dec 28, 2017 • edited Loading

kimjoar commented Feb 5, 2018

kimjoar Dec 5, 2017 •

edited

Loading

spalger commented Dec 28, 2017 •

edited

Loading