Respect EditorConfig settings by josephfrazier · Pull Request #2760 · prettier/prettier

josephfrazier · 2017-09-06T01:29:00Z

This is a simple way to address #42. It adds support for .editorconfig's indent_style, tab_width, and max_line_length properties.

I still need to add caching and a proper async implementation, but I think it's functionally correct. It doesn't support the end_of_line property as described in #42 (comment), but that could be added later.

This is a simple way to address prettier#42. It adds support for .editorconfig's `indent_style`, `tab_width`, and `max_line_length` properties. I still need to add caching and a proper async implementation, but I think it's functionally correct. It doesn't support the `end_of_line` property as described in prettier#42 (comment), but that could be added later.

lydell · 2017-09-06T05:20:14Z

@@ -0,0 +1,6 @@
+root = true
+
+[*]


We should test a more specific glob as well, with different settings, like [lib/**.js] (copied from http://editorconfig.org/).

See prettier#2760 (review)

josephfrazier · 2017-09-06T12:40:04Z

Good call, it looks like that exposed a bug as well. I still need to fix the pre-existing tests that now fail.

The tests were failing because they glob over all config-resolution test files.

lydell · 2017-09-06T18:00:00Z

@@ -0,0 +1,5 @@
+/* eslint-disable */


eslint-disable comments shouldn't be needed because we ignore test_integration/cli/ in .eslintignore

lydell · 2017-09-06T18:14:45Z

+  result.useTabs = editorConfig.indent_style === "tab";
+  result.tabWidth =
+    editorConfig.indent_size || editorConfig.tab_width || result.tabWidth;
+  result.printWidth = editorConfig.max_line_length || result.printWidth;


There's no need for || result.tabWidth and || result.printWidth, is it? They are always goind to be undefined.

Couldn't we just do:

const editorConfig = editorconfig.parseSync(filePath); return { useTabs: ..., tabWidth: ..., printWidth: ... }

While being an odd configuration, isn't it valid to set indent_size, tab_width and max_line_length to 0, and as such we should respect that?

There's no need for || result.tabWidth and || result.printWidth, is it? They are always goind to be undefined.

Yeah, that was also causing a bug. Not sure why I did it that way, but it's fixed in 54a5dcb

While being an odd configuration, isn't it valid to set indent_size, tab_width and max_line_length to 0, and as such we should respect that?

Good point, I'll try to fix that.

azz · 2017-09-07T02:11:35Z

I'm still not 100% convinced we should add this, ~~as editors that use the prettier API (prettier-vscode, prettier-atom) will still have to implement it themselves.~~

edit: never mind, they get it from resolveConfig()

See prettier#2760 (comment)

lydell · 2017-09-07T05:14:18Z

+  if (editorConfig.max_line_length) {
+    result.printWidth = editorConfig.max_line_length;
+  }
+


Is it important that the .useTabs, .tabWidth and .printWidth properties of result are missing rather than present with the value undefined if the corresponding EditorConfig setting is missing?

Couldn't we just do:

const editorConfig = editorconfig.parseSync(filePath); return { useTabs: ..., tabWidth: ..., printWidth: ... }

While being an odd configuration, isn't it valid to set indent_size, tab_width and max_line_length to 0, and as such we should respect that? Note that 0 if a falsy value.

Is it important that the .useTabs, .tabWidth and .printWidth properties of result are missing rather than present with the value undefined if the corresponding EditorConfig setting is missing?

Yes, I think that's what fixed this test: https://travis-ci.org/prettier/prettier/jobs/272596635#L379

While being an odd configuration, isn't it valid to set indent_size, tab_width and max_line_length to 0, and as such we should respect that? Note that 0 if a falsy value.

Hmm, it looks like tab_width and max_line_length must be positive. I suppose someone could set indent_size to 0, but I'm not sure if that edge case is worth supporting right now. At least, I haven't had time to put effort into it.

I guess it's all fine then 👍

lydell · 2017-09-11T18:16:03Z

@josephfrazier Is this still WIP or ready to merge?

josephfrazier · 2017-09-11T18:37:12Z

I haven't had time to add a proper async implementation, as well as caching. I think there might also be an bug in the case where there's no prettier config, but there is .editorconfig, but I'm not sure yet.

EDIT: I think this is ready now.

From https://github.com/davidtheclark/cosmiconfig/tree/bf5cc1aff958090e73c97be1550da3312c972b35#usage: > If no configuration object is found, the `cosmiconfig()` Promise resolves with `null`.

lydell

I like the implementation! Really easy to follow.

I left two small comments. Otherwise looks good to me.

lydell · 2017-09-12T05:55:14Z


 test("API resolveConfig.sync with no args", () => {
-  expect(prettier.resolveConfig.sync()).toBeNull();
+  expect(prettier.resolveConfig.sync()).toEqual({});


Hmm ... this (and the change just above) are breaking changes – is that something we want to avoid?

lydell · 2017-09-12T05:55:46Z

+};
+const editorconfigAsyncWithCache = mem(editorconfigAsyncNoCache);
+const editorconfigSyncNoCache = filePath =>
+  filePath && editorConfigToPrettier(editorconfig.parseSync(filePath));


This arrow function has no braces while the above when has.

See prettier#2760 (comment)

lipis · 2017-10-30T17:16:53Z

What's the latest on this one?! (I don't mean to rush or anything.. just curious)

josephfrazier · 2017-10-30T20:51:30Z

@lipis, I'd still like to address @robwise's concerns in #2760 (comment), but I haven't had time to do so. While investigating that, I also noticed that resolve-config-editorconfig prioritizes indent_size over tab_width, which is exactly backwards according to http://editorconfig.org/:

tab_width: a whole number defining the number of columns used to represent a tab character. This defaults to the value of indent_size and doesn't usually need to be specified.

If you (or anyone else) would like to help out, please feel free to post patches (preferably wrapped in a <details> element) that I can test and add to the PR.

…_width prettier#2760 (comment)

…as tabWidth

prettier#2760 (comment)

josephfrazier · 2017-11-02T00:38:08Z

@robwise, I think your aforementioned concerns are now addressed. Would you mind taking another look and confirming?

This makes it easier to reason about, test, and iterate upon. https://github.com/josephfrazier/editorconfig-to-prettier/tree/bf25f41c3defe3ef046ed0144d569fc6f92c9b81

josephfrazier · 2017-11-08T19:32:33Z

After having to resolve merge conflicts with master yet again, I decided to go ahead and land this, especially since I believe all of the previously mentioned concerns have been adequately addressed.

azz · 2017-11-08T21:07:19Z

Not sure adding this in a hotfix is a good idea...

This reverts commit 8f58ca0.

lipis · 2017-11-08T21:22:36Z

Good stuff:)

josephfrazier · 2017-11-08T21:45:56Z

Oops @azz, I didn't realize that the next release was intended to be only a hotfix. What do you think about backing this out (#3213), releasing 1.8.2, then landing it again? It's been a rather long-running PR, so I'd really like to get it in.

As far as I can tell, the only projects this change could break are those that have .editorconfig properties without the corresponding .prettierrc options. Prettier would now take into account the extra .editorconfig properties, whereas before it wouldn't. This doesn't seem like such a big deal to me, considering that projects should edit/remove .editorconfig files that are intentionally disregarded when the code is checked in.

* Revert "Respect EditorConfig settings (#2760)" This reverts commit 8f58ca0.

See prettier#2760 (comment) and josephfrazier/editorconfig-to-prettier@c38b84c

* Revert "Revert "Respect EditorConfig settings" (#3213)" This reverts commit d2241fc. * Comment out EditorConfig docs See #3213 (comment) * editorconfig: Support `indent_size = 0` See #2760 (comment) and josephfrazier/editorconfig-to-prettier@c38b84c * Revert "Comment out EditorConfig docs" This reverts commit ddfa529. * Mark EditorConfig functionality as v1.9.0+ See #3255 (comment) * editorconfig: Upgrade editorconfig-to-prettier to 0.0.4 * editorconfig: Only enable for CLI, by default #3255 (comment) * editorconfig: Add tests confirming that editorconfig is ignored by default in the API #3255 (comment) * editorconfig: Add/fix CLI option parsing * editorconfig: Move docs from configuration.md to options.md * editorconfig: Add `oppositeDescription` to show docs for `--no-editorconfig` Addresses #3255 (comment) * editorconfig: Update test snapshots * editorconfig: Remove unnecessary options parsing code Addresses #3255 (comment) * editorconfig: Move docs from options.md to api.md and cli.md Addresses #3255 (comment) * resolveConfig: return null if both .prettierrc and .editorconfig are missing Addresses #3255 (comment) * Don't add now-failing tests The way these tests work, both `tests_integration/cli/config/.prettierrc` and `.prettierrc` apply to `tests_integration/cli/config/editorconfig/file.shouldnotexist`, so the test wouldn't work even on master. Here's a way to confirm that: ```js const path = require('path') const assert = require('assert') const prettier = require('./') const file = './tests_integration/cli/config/editorconfig/file.shouldnotexist' console.log(prettier.resolveConfig.sync(file)) assert(prettier.resolveConfig.sync(file) === null) ```

josephfrazier added 2 commits September 5, 2017 21:26

Merge branch 'master' into editorconfig

6897f6b

lydell reviewed Sep 6, 2017

View reviewed changes

Add test for more specific .editorconfig glob

24821d5

See prettier#2760 (review)

Update config-resolution test snapshots

9a7a17a

The tests were failing because they glob over all config-resolution test files.

lydell reviewed Sep 6, 2017

View reviewed changes

josephfrazier added 2 commits September 6, 2017 14:02

Fix lint

7d92baf

fixup! Update config-resolution test snapshots

2a1ae88

lydell reviewed Sep 6, 2017

View reviewed changes

darahak mentioned this pull request Sep 6, 2017

Implement Prettier Config prettier/prettier-atom#261

Merged

josephfrazier added 2 commits September 6, 2017 23:13

Ignore absent EditorConfig settings

54a5dcb

Remove unnecessary eslint-disable comments

4f52651

See prettier#2760 (comment)

lydell reviewed Sep 7, 2017

View reviewed changes

josephfrazier added 2 commits September 8, 2017 21:27

Merge branch 'master' into editorconfig

ad69825

WIP extract helper function

bf534cd

josephfrazier self-assigned this Sep 11, 2017

josephfrazier added 7 commits September 11, 2017 21:58

Move editorconfig parsing outside helper

53e6f17

use async editorconfig

4f660e6

parallelize async resolveConfig

ccd03c6

Merge branch 'master' into editorconfig

5e6ef51

Add caching for editorconfig

a42fa75

Fix syntax for Node v4

f123c64

lydell reviewed Sep 12, 2017

View reviewed changes

josephfrazier added 2 commits September 12, 2017 08:40

Remove unnecessary braces from arrow function

8404ee0

See prettier#2760 (comment)

Return null from resolveConfig() when filePath isn't provided

85e350b

See prettier#2760 (comment)

robwise mentioned this pull request Oct 20, 2017

Prettier-atom not respecting quote_type directive from EditorConfig prettier/prettier-atom#293

Closed

josephfrazier added 4 commits November 1, 2017 20:13

editorconfig: test indent_style = tab overriding indent_size with tab…

db9877c

…_width prettier#2760 (comment)

editorconfig: prioritize tab_width over indent_size

a9bc09e

editorconfig: let indent_style force either tab_width or indent_size …

26f49a4

…as tabWidth

editorconfig: test/fix indent_size = tab

bf282f9

prettier#2760 (comment)

josephfrazier added 4 commits November 1, 2017 22:12

Move editorConfigToPrettier into separate package

27254d4

This makes it easier to reason about, test, and iterate upon. https://github.com/josephfrazier/editorconfig-to-prettier/tree/bf25f41c3defe3ef046ed0144d569fc6f92c9b81

Merge branch 'master' into editorconfig

5526de0

Merge branch 'master' into editorconfig

664dced

Merge branch 'master' into josephfrazier-editorconfig

47e4b62

josephfrazier merged commit 8f58ca0 into prettier:master Nov 8, 2017

azz added a commit that referenced this pull request Nov 8, 2017

Revert "Respect EditorConfig settings (#2760)"

5174a6d

This reverts commit 8f58ca0.

azz mentioned this pull request Nov 8, 2017

Revert "Respect EditorConfig settings" #3213

Merged

josephfrazier mentioned this pull request Nov 8, 2017

tab_width=0? josephfrazier/editorconfig-to-prettier#1

Closed

azz added a commit that referenced this pull request Nov 9, 2017

Revert "Respect EditorConfig settings" (#3213)

d2241fc

* Revert "Respect EditorConfig settings (#2760)" This reverts commit 8f58ca0.

josephfrazier mentioned this pull request Nov 12, 2017

Re-add EditorConfig support (undo #3213) #3255

Merged

josephfrazier added a commit to josephfrazier/prettier that referenced this pull request Nov 12, 2017

editorconfig: Support indent_size = 0

6dbd67f

See prettier#2760 (comment) and josephfrazier/editorconfig-to-prettier@c38b84c

josephfrazier mentioned this pull request Dec 11, 2017

Expose config.resolveConfigFile in API? #3451

Closed

josephfrazier deleted the editorconfig branch January 30, 2018 17:40

josephfrazier restored the editorconfig branch February 12, 2018 23:10

josephfrazier deleted the editorconfig branch June 12, 2018 15:52

lock Bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Sep 10, 2018

lock Bot locked as resolved and limited conversation to collaborators Sep 10, 2018

Uh oh!

Conversation

josephfrazier commented Sep 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josephfrazier commented Sep 6, 2017 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azz commented Sep 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lydell Sep 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lydell commented Sep 11, 2017

Uh oh!

josephfrazier commented Sep 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lydell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lipis commented Oct 30, 2017

Uh oh!

josephfrazier commented Oct 30, 2017

Uh oh!

josephfrazier commented Nov 2, 2017

Uh oh!

josephfrazier commented Nov 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

azz commented Nov 8, 2017

Uh oh!

lipis commented Nov 8, 2017

Uh oh!

josephfrazier commented Nov 8, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

josephfrazier commented Sep 6, 2017 •

edited

Loading

josephfrazier commented Sep 6, 2017 via email •

edited

Loading

azz commented Sep 7, 2017 •

edited

Loading

lydell Sep 7, 2017 •

edited

Loading

josephfrazier commented Sep 11, 2017 •

edited

Loading

josephfrazier commented Nov 8, 2017 •

edited

Loading