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

josephfrazier · 2017-11-12T18:57:10Z

Reintroducing #2760 for the 1.9 release, as suggested in #3213 (comment)

This reverts commit d2241fc.

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

ikatyang · 2017-11-13T01:08:31Z

docs/configuration.md

@@ -83,3 +83,13 @@ For more information on how to use the CLI to locate a file, see the [CLI](cli.m
 ## Configuration Schema

 If you'd like a JSON schema to validate your configuration, one is available here: http://json.schemastore.org/prettierrc.
+
+<!--
+  ## EditorConfig


I think we can add an _available in v1.9.0+_ note just like https://prettier.io/docs/en/options.html#prose-wrap, so that we don't have to uncomment new feature every time and it's clearer which version is available for it.

This reverts commit ddfa529.

See prettier#3255 (comment)

lipis · 2017-11-16T15:16:30Z

Will the following work out of the box, just by running Prettier on all supported files?

root = true

[*]
indent_style = space
indent_size = 2
max_line_length = 80

[*.{js,jsx}]
max_line_length = 120

[*.json]
indent_size = 4
max_line_length = 200

josephfrazier · 2017-11-16T15:50:56Z

@lipis, I think so! If you have a clone of the prettier repo, you can test this branch using hub:

hub checkout https://github.com/prettier/prettier/pull/3255
yarn
./bin/prettier.js /the/globs/of/your/files

lipis · 2017-11-16T16:15:46Z

Maybe we should support filetypes inside the .prettierrc somehow.. would be awesome :) Maybe I should create a new issue for that though..

josephfrazier · 2017-11-16T16:17:21Z

They are supported :) See https://github.com/prettier/prettier/blob/258de9dac85f4afff3ebcf40057622ec5a82b113/docs/configuration.md#configuration-overrides

lipis · 2017-11-16T16:24:17Z

WHAT?! Nice.. thanks :) RTFM :D

josephfrazier · 2017-11-16T17:13:49Z

You're welcome! If you do test this branch with the .editorconfig file you posted above, please let me know how it goes.

lipis · 2017-11-16T21:54:05Z

I will once I'll get home.. :)

azz · 2017-11-21T11:19:47Z

src/resolve-config.js

+      mergeOverrides(Object.assign({}, result), filePath)
+    );
+
+    if (Object.keys(merged).length === 0) {


This doesn't look the same as the current behaviour. We want an empty config file to be different than no config file. How this works with .editorconfig... I don't know. Editors want to support formatting only if a config file is found, so supporting EditorConfig might break that functionality.

This doesn't look the same as the current behaviour.

According to @robwise in #2760 (comment), the current behavior treats missing/empty config files equivalently. I wanted to be sure, so here's a script you can run against a clean build of cbed0f4 to try it out:

tmpdir=`mktemp -d` tmpfile=$tmpdir/file.js tmprc=$tmpdir/.prettierrc echo -n > $tmpfile echo "With no .prettierrc" node -p "require('./').resolveConfig.sync('$tmpfile')" echo echo "With empty .prettierrc" echo -n > $tmprc node -p "require('./').resolveConfig.sync('$tmpfile')" echo rm -rf $tmpdir

On my machine, this prints:

With no .prettierrc null With empty .prettierrc null

Because of this, I'd like to fix this missing/empty distinction in a separate PR, since it's pre-existing and (somewhat?) unrelated (see #2760 (comment))

How this works with .editorconfig... I don't know. Editors want to support formatting only if a config file is found, so supporting EditorConfig might break that functionality.

Hmm, I guess the use-case here is:

The user has a prettier integration in their editor

The integration is configured to format-on-save if a prettier configuration exists

The user is working on a project that has a .editorconfig

But the project's style guide is incompatible with prettier's formatting

Yeah, I could see how that might get tricky. I'm going to have to give it some more thought.

Yeah, I could see how that might get tricky. I'm going to have to give it some more thought.

Ok, so one way to fix this is to make it so that resolveConfigFile() (and its CLI counterpart --find-config-path) return paths to empty .prettierrc files (it doesn't currently). Once that's the case, the editor integration can use it to determine whether an empty .prettierrc exists, even if resolveConfig returns an object derived from .editorconfig.

Using resolveConfigFile(path) != null could be a solution.
I've not seen this function exposed for now, seems to be cli only.

Ok, so one way to fix this is to make it so that resolveConfigFile() (and its CLI counterpart --find-config-path) return paths to empty .prettierrc files (it doesn't currently).

Hmm, it looks like cosmiconfig intentionally ignores empty files: cosmiconfig/cosmiconfig@1adf5c2

However, a .prettierrc file containing just a space or a newline is still found, so that could be the suggested workaround instead of this from #2760 (comment):

azz

⬆️

lipis · 2017-11-27T21:41:13Z

Are we there yet?! It works :)

josephfrazier · 2017-11-27T21:45:16Z

I think there's still a risk that some editor integrations could break under certain circumstances (see #3255 (comment) for details). Besides, we also want to make sure that there are no more patch releases before 1.9.0, so this is kinda just chilling for now.

josephfrazier · 2017-11-30T15:54:26Z

It sounds like 1.9.0 is going to be the next release (#3333 (comment)). @azz, do you have any thoughts on #3255 (comment)?

azz · 2017-12-01T07:18:53Z

How about this:

Add a editorconfig: true|false to the options of resolveConfig to disable lookup of editorconfig. Default to false for backwards compat, but set it to true internally from the CLI.

josephfrazier · 2017-12-01T17:41:52Z

That sounds like a great way to sidestep potential editor integration issues, while still making prettier friendly to new users that use EditorConfig in their projects! I'll see what I can do

azz · 2017-12-03T04:03:19Z

Looks like this is the last one left in the 1.9 milestone. Did you get a chance to make that change?

josephfrazier · 2017-12-03T17:48:24Z

It's still in-progress. I think there are a few more things in the milestone, though? https://github.com/prettier/prettier/milestone/7

prettier#3255 (comment)

…fault in the API prettier#3255 (comment)

azz · 2017-12-03T21:09:58Z

#3379 has since been added. The rest are post release items

azz · 2017-12-04T22:31:00Z

Sorry this took so long to get in!

lipis · 2017-12-04T22:32:13Z

KaBoom! 💥 🎆

josephfrazier · 2017-12-05T02:08:30Z

🎉 It's understandable, @azz. This turned out to be a tricky change to get right. I'll see if I can add a little more detail to the release draft.

See prettier#3255 (comment)

* Respect EditorConfig when `--config` is passed * Clarify config-resolution test file See #3255 (comment) * Update test snapshots

josephfrazier added 2 commits November 12, 2017 13:45

Revert "Revert "Respect EditorConfig settings" (prettier#3213)"

9892602

This reverts commit d2241fc.

Comment out EditorConfig docs

ddfa529

See prettier#3213 (comment)

josephfrazier added this to the 1.9 milestone Nov 12, 2017

editorconfig: Support indent_size = 0

6dbd67f

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

ikatyang reviewed Nov 13, 2017

View reviewed changes

josephfrazier added 2 commits November 12, 2017 20:30

Revert "Comment out EditorConfig docs"

b2637d7

This reverts commit ddfa529.

Mark EditorConfig functionality as v1.9.0+

2f643e9

See prettier#3255 (comment)

azz reviewed Nov 21, 2017

View reviewed changes

azz previously requested changes Nov 21, 2017

View reviewed changes

josephfrazier added 2 commits November 22, 2017 09:00

Merge branch 'master' into editorconfig-2

aac81c4

editorconfig: Upgrade editorconfig-to-prettier to 0.0.4

d0beb79

duailibe mentioned this pull request Nov 28, 2017

1.9 Release #3333

Closed

5 tasks

josephfrazier added 3 commits December 3, 2017 13:17

editorconfig: Only enable for CLI, by default

bcf6f03

prettier#3255 (comment)

editorconfig: Add tests confirming that editorconfig is ignored by de…

f9c8cc3

…fault in the API prettier#3255 (comment)

Merge branch 'master' into editorconfig-2

d5dcbfa

Merge branch 'master' into editorconfig-2

f74aa23

azz merged commit cecf065 into prettier:master Dec 4, 2017

dependencies bot mentioned this pull request Dec 5, 2017

Update 2 dependencies from npm storybookjs/storybook#2430

Merged

josephfrazier mentioned this pull request Dec 14, 2017

Can we enable the dangling comma on this project? #3469

Closed

dependencies bot mentioned this pull request Dec 27, 2017

Update prettier in / from 1.8.2 to 1.9.2 dailybruin/sources#18

Merged

This was referenced Feb 8, 2018

Add prettierrc to prettier source code #3885

Closed

Why config implicitly ignores editorconfig? #3963

Closed

josephfrazier added a commit to josephfrazier/prettier that referenced this pull request Feb 17, 2018

Clarify config-resolution test file

7c27f1b

See prettier#3255 (comment)

josephfrazier mentioned this pull request Feb 17, 2018

Respect EditorConfig when --config is passed #3992

Merged

dependencies bot mentioned this pull request Feb 19, 2018

prettier versions available: 1.10.2, 1.10.1, 1.10.0, 1.9.2, 1.9.1, 1.9.0 serlo/serlo-abc#257

Closed

azz pushed a commit that referenced this pull request Feb 24, 2018

Respect EditorConfig when --config is passed (#3992)

5153b15

* Respect EditorConfig when `--config` is passed * Clarify config-resolution test file See #3255 (comment) * Update test snapshots

lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jan 19, 2019

lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019

josephfrazier deleted the editorconfig-2 branch January 2, 2020 00:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

josephfrazier commented Nov 12, 2017

ikatyang Nov 13, 2017

lipis commented Nov 16, 2017 •

edited

josephfrazier commented Nov 16, 2017

lipis commented Nov 16, 2017

josephfrazier commented Nov 16, 2017

lipis commented Nov 16, 2017 •

edited

josephfrazier commented Nov 16, 2017

lipis commented Nov 16, 2017

azz Nov 21, 2017

josephfrazier Nov 21, 2017

josephfrazier Nov 21, 2017

CiGit Nov 21, 2017

josephfrazier Nov 21, 2017 •

edited

azz left a comment •

edited

lipis commented Nov 27, 2017

josephfrazier commented Nov 27, 2017

josephfrazier commented Nov 30, 2017

azz commented Dec 1, 2017

josephfrazier commented Dec 1, 2017

azz commented Dec 3, 2017

josephfrazier commented Dec 3, 2017

azz commented Dec 3, 2017

azz commented Dec 4, 2017

lipis commented Dec 4, 2017

josephfrazier commented Dec 5, 2017

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

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

Conversation

josephfrazier commented Nov 12, 2017

ikatyang Nov 13, 2017

Choose a reason for hiding this comment

lipis commented Nov 16, 2017 • edited

josephfrazier commented Nov 16, 2017

lipis commented Nov 16, 2017

josephfrazier commented Nov 16, 2017

lipis commented Nov 16, 2017 • edited

josephfrazier commented Nov 16, 2017

lipis commented Nov 16, 2017

azz Nov 21, 2017

Choose a reason for hiding this comment

josephfrazier Nov 21, 2017

Choose a reason for hiding this comment

josephfrazier Nov 21, 2017

Choose a reason for hiding this comment

CiGit Nov 21, 2017

Choose a reason for hiding this comment

josephfrazier Nov 21, 2017 • edited

Choose a reason for hiding this comment

azz left a comment • edited

Choose a reason for hiding this comment

lipis commented Nov 27, 2017

josephfrazier commented Nov 27, 2017

josephfrazier commented Nov 30, 2017

azz commented Dec 1, 2017

josephfrazier commented Dec 1, 2017

azz commented Dec 3, 2017

josephfrazier commented Dec 3, 2017

azz commented Dec 3, 2017

azz commented Dec 4, 2017

lipis commented Dec 4, 2017

josephfrazier commented Dec 5, 2017

lipis commented Nov 16, 2017 •

edited

lipis commented Nov 16, 2017 •

edited

josephfrazier Nov 21, 2017 •

edited

azz left a comment •

edited