Re-add EditorConfig support (undo #3213)#3255
Conversation
| If you'd like a JSON schema to validate your configuration, one is available here: http://json.schemastore.org/prettierrc. | ||
|
|
||
| <!-- | ||
| ## EditorConfig |
There was a problem hiding this comment.
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.
|
Will the following work out of the box, just by running Prettier on all supported files? |
|
Maybe we should support filetypes inside the |
|
WHAT?! Nice.. thanks :) RTFM :D |
|
You're welcome! If you do test this branch with the |
|
I will once I'll get home.. :) |
| mergeOverrides(Object.assign({}, result), filePath) | ||
| ); | ||
|
|
||
| if (Object.keys(merged).length === 0) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 $tmpdirOn 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Using resolveConfigFile(path) != null could be a solution.
I've not seen this function exposed for now, seems to be cli only.
There was a problem hiding this comment.
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):
|
Are we there yet?! It works :) |
|
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. |
|
It sounds like 1.9.0 is going to be the next release (#3333 (comment)). @azz, do you have any thoughts on #3255 (comment)? |
|
How about this: Add a |
|
That sounds like a great way to sidestep potential editor integration issues, while still making |
|
Looks like this is the last one left in the 1.9 milestone. Did you get a chance to make that change? |
|
It's still in-progress. I think there are a few more things in the milestone, though? https://github.com/prettier/prettier/milestone/7 |
|
#3379 has since been added. The rest are post release items |
|
Sorry this took so long to get in! |
|
KaBoom! 💥 🎆 |
|
🎉 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. |
* Respect EditorConfig when `--config` is passed * Clarify config-resolution test file See #3255 (comment) * Update test snapshots
Reintroducing #2760 for the 1.9 release, as suggested in #3213 (comment)
This reverts commit d2241fc.