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 opt-in prettier #15106

Closed
wants to merge 7 commits into from
Closed

Add opt-in prettier #15106

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a4eaf9f
Add opt-in prettier
Nov 22, 2017
0f9ffb0
ignore some files wherever they are
Nov 22, 2017
8fbb538
use std settings for prettier and cleaned up package.json
Nov 22, 2017
a3938b7
Merge branch 'master' into add-prettier
mattapperson Nov 27, 2017
13d64de
cleanup package.json
Nov 27, 2017
95790f2
update docs to mention prettier and prettier setup
mattapperson Nov 27, 2017
b093d13
Added prettier as a node script vs npm script
Nov 27, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 57 additions & 0 deletions .prettierignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
node_modules
.git
package.json

/bin/*
/config/*
/data/*
/docs/*
/optimize/*
/packages/*
/plugins/*
/scripts/*
/style_guides/*
/tasks/*
/test/*
/ui_framework/*
/utilities/*
/webpackShims/*
/GruntFile.js

/src/babel-preset/*
/src/babel-register/*
/src/cli/*
/src/cli_keystore/*
/src/cli_plugin/*
/src/deprecation/*
/src/dev/*
/src/docs/*
/src/es_archiver/*
/src/fixtures/*
/src/functional_test_runner/*
/src/jest/*
/src/optimize/*
/src/server/*
/src/test_utils/*
/src/ui/*
/src/utils/*

/src/core_plugins/console/*
/src/core_plugins/dev_mode/*
/src/core_plugins/elasticsearch/*
/src/core_plugins/input_control_vis/*
/src/core_plugins/kbn_doc_views/*
/src/core_plugins/kbn_vislib_vis_types/*
/src/core_plugins/kibana/*
/src/core_plugins/markdown_vis/*
/src/core_plugins/metric_vis/*
/src/core_plugins/region_map/*
/src/core_plugins/spy_modes/*
/src/core_plugins/state_session_storage_redirect/*
/src/core_plugins/status_page/*
/src/core_plugins/table_vis/*
/src/core_plugins/tagcloud/*
/src/core_plugins/testbed/*
/src/core_plugins/tests_bundle/*
/src/core_plugins/tile_map/*
/src/core_plugins/timelion/*
4 changes: 4 additions & 0 deletions .prettierrc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"singleQuote": true,
"semi": true
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah cool, yea I missed that comment.

}
21 changes: 11 additions & 10 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,8 @@
{
"name": "kibana",
"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"
],
"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"],
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: revert these changes ^?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

"private": false,
"version": "7.0.0-alpha1",
"branch": "master",
Expand All @@ -21,6 +14,10 @@
"bugs": {
"url": "http://github.com/elastic/kibana/issues"
},
"pre-commit": ["pre-commit"],
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 ?

"lint-staged": {
"*.js": ["prettier --write", "git add"]
},
"license": "Apache-2.0",
"author": "Rashid Khan <rashid.khan@elastic.co>",
"contributors": [
Expand Down Expand Up @@ -54,6 +51,7 @@
"build": "grunt build",
"release": "grunt release",
"start": "sh ./bin/kibana --dev",
"prettier": "./node_modules/prettier/bin/prettier.js \"./**/*.js\" --write",
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gtk thanks!

"precommit": "node scripts/precommit_hook",
"karma": "karma start",
"elasticsearch": "grunt esvm:dev:keepalive",
Expand All @@ -62,6 +60,7 @@
"makelogs": "echo 'use `node scripts/makelogs`' && false",
"mocha": "echo 'use `node scripts/mocha`' && false",
"sterilize": "grunt sterilize",
"pre-commit": "lint-staged",
"uiFramework:start": "grunt uiFramework:start",
"uiFramework:build": "grunt uiFramework:build",
"uiFramework:createComponent": "yo ./ui_framework/generator-kui/app/component.js",
Expand Down Expand Up @@ -215,6 +214,8 @@
"yauzl": "2.7.0"
},
"devDependencies": {
"lint-staged": "5.0.0",
"prettier": "1.8.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

"@elastic/eslint-config-kibana": "0.14.0",
"@elastic/eslint-import-resolver-kibana": "0.9.0",
"@elastic/eslint-plugin-kibana-custom": "1.1.0",
Expand Down