Prettier #4012

reichhartd · 2018-07-03T20:36:24Z

PR checklist

Addresses an existing issue: #0000
New feature, bugfix, or enhancement
- Includes tests
Documentation update

Overview of change:

Change "npm run" to "yarn" in docs/develop/docs/index.md
Added the code formatter prettier (https://prettier.io/)
Added a precommit hook: To run prettier pre commit over every staged file

Is there anything you'd like reviewers to focus on?

CHANGELOG.md entry:

palantirtech · 2018-07-03T20:36:26Z

Thanks for your interest in palantir/tslint, @danielreichhart! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

Shinigami92 · 2018-07-04T03:20:35Z

package.json

    "json-stringify-pretty-compact": "^1.0.3",
    "mocha": "^3.2.0",
    "npm-run-all": "^4.0.2",
    "nyc": "^10.2.0",
+    "prettier": "^1.13.7",


Like the documentation pointed out https://prettier.io/docs/en/install.html
Use exact version instead of ^ or ~
Only use 1.13.7

Right. I changed it.

Shinigami92 · 2018-07-04T03:25:27Z

.gitignore

@@ -26,3 +26,6 @@ docs/formatters/*/index.html

 /coverage/
 /.nyc_output
+
+# ide


Not a huge problem here, but try to do only one thing in a PR
The repository owner can not cerry-pick like in GitLab (in the GitHub GUI)

pablobirukov · 2018-07-04T11:34:51Z

Why is prettier needed? From my expirience it makes development less convenient (I'm talking only about typescript):

in some cases it reduces readabiility
maked PR management more complicated because it puts arguments and imported members on a single line or multiple lines depending on line length. For example, if you just rename an argument and change its name's length, instead of changing one line, prettier will change several lines, as
```
function foo(
  does,
  not,
  fit,
)
// ↓
function foo(do, fit, now)
```

reichhartd · 2018-07-04T12:18:04Z

Okay, that’s right. But in my opinion, it reduces the readability only in few edge cases. In general, it improves it. For the few cases, we could open an issue or a pull request at the prettier project.

You can define the line length in the .prettierrc config file. The current print width is 120 chars. If it makes the PR management more complicated, we could increase the length.

johnwiseheart · 2018-07-05T17:45:38Z

I agree that prettier is a good idea to add here: in the majority of cases, it simplifies code style and makes it easier to review PRs - there no longer needs to be checks of code layout/formatting as prettier enforces this. @danielreichhart any idea why the tests are failing after this change? It might not be an issue with this PR but we should figure it out before merging.

reichhartd · 2018-07-09T16:48:44Z

It is a issue with some older pull requests, the failing tests are fixed here #4011 #4019

johnwiseheart · 2018-07-11T22:53:44Z

I merged the PR for the failing tests - could you merge master into your PR and re-push?

# Conflicts: # yarn.lock

reichhartd · 2018-07-12T13:14:19Z

@johnwiseheart done

johnwiseheart · 2018-07-12T19:43:05Z

.prettierrc.json

@@ -0,0 +1,4 @@
+{
+    "tabWidth": 4,
+    "printWidth": 120


I'm not sure about having a print width of 120 - the prettier docs specifically mention why the default of 80 makes sense. @giladgray do you have any comments here? Maybe we could set it to 100 so its in the middle.

@johnwiseheart @giladgray are there any news?

IMO 100 is a good choice because you can see the changes in a commit side by side without scrolling (on a 1080p monitor)

I’ve always worked with 120, but 100 is a good compromise.

Also, the point the commit changes without scrolling is a good point.

johnwiseheart · 2018-07-23T20:25:46Z

Thanks for contributing!

palantir#4012 added prettier support but then didn't run it over everything and turned it on for YAML files despite using a version that doesn't support YAML. This: * Upgrades Prettier to 1.14 whic adds yml support. * Turns on `trailing commas = all` as that seems to be what most of the code is doing. * Runs prettier over everything This also has the bonus of making future PRs much smaller.

reichhartd added 8 commits July 3, 2018 21:56

Changed npm run to yarn in docs

796a28e

Added ignore line for intellij specific settings

0a92b13

Added prettier

038ea15

Added prettier config file

a4ebcce

Added tslint-config-prettier

2588893

Added pre-commit hook prettier

10d3ad0

Added prettier ignore file

df6ce78

Changed tab width for package.json, to see the changes better

1457b8e

Shinigami92 reviewed Jul 4, 2018

View reviewed changes

Pinned exact version in package.json

bf249a4

Reichhart, Daniel added 2 commits July 12, 2018 14:59

Merge branch 'master' into prettier

8b540d5

# Conflicts: # yarn.lock

Merged yarn.lock

57bc3ff

johnwiseheart reviewed Jul 12, 2018

View reviewed changes

Changed printWidth to 100

4a8f769

johnwiseheart approved these changes Jul 23, 2018

View reviewed changes

johnwiseheart merged commit 3617be0 into palantir:master Jul 23, 2018

Shinigami92 mentioned this pull request Jul 28, 2018

[new-rule-option] no-space-keywords for comment-format #4003

Closed

4 tasks

DJMcNab mentioned this pull request Jul 31, 2018

fix(rules): remove no-unused-variable from the 'tslint:all' config #4079

Merged

3 tasks

aboyton mentioned this pull request Oct 9, 2018

Upgrade prettier and run over all files #4214

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prettier #4012

Prettier #4012

reichhartd commented Jul 3, 2018 •

edited

Loading

palantirtech commented Jul 3, 2018

Shinigami92 Jul 4, 2018

reichhartd Jul 4, 2018

Shinigami92 Jul 4, 2018

pablobirukov commented Jul 4, 2018

reichhartd commented Jul 4, 2018 •

edited

Loading

johnwiseheart commented Jul 5, 2018 •

edited

Loading

reichhartd commented Jul 9, 2018 •

edited

Loading

johnwiseheart commented Jul 11, 2018

reichhartd commented Jul 12, 2018

johnwiseheart Jul 12, 2018

reichhartd Jul 21, 2018

Shinigami92 Jul 21, 2018

reichhartd Jul 21, 2018

johnwiseheart commented Jul 23, 2018

Prettier #4012

Prettier #4012

Conversation

reichhartd commented Jul 3, 2018 • edited Loading

PR checklist

Overview of change:

Is there anything you'd like reviewers to focus on?

CHANGELOG.md entry:

palantirtech commented Jul 3, 2018

Shinigami92 Jul 4, 2018

Choose a reason for hiding this comment

reichhartd Jul 4, 2018

Choose a reason for hiding this comment

Shinigami92 Jul 4, 2018

Choose a reason for hiding this comment

pablobirukov commented Jul 4, 2018

reichhartd commented Jul 4, 2018 • edited Loading

johnwiseheart commented Jul 5, 2018 • edited Loading

reichhartd commented Jul 9, 2018 • edited Loading

johnwiseheart commented Jul 11, 2018

reichhartd commented Jul 12, 2018

johnwiseheart Jul 12, 2018

Choose a reason for hiding this comment

reichhartd Jul 21, 2018

Choose a reason for hiding this comment

Shinigami92 Jul 21, 2018

Choose a reason for hiding this comment

reichhartd Jul 21, 2018

Choose a reason for hiding this comment

johnwiseheart commented Jul 23, 2018

reichhartd commented Jul 3, 2018 •

edited

Loading

reichhartd commented Jul 4, 2018 •

edited

Loading

johnwiseheart commented Jul 5, 2018 •

edited

Loading

reichhartd commented Jul 9, 2018 •

edited

Loading