Respect EditorConfig settings #2760
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,31 +1,76 @@ | ||
| "use strict"; | ||
|
|
||
| const cosmiconfig = require("cosmiconfig"); | ||
| const editorconfig = require("editorconfig"); | ||
| const mem = require("mem"); | ||
| const minimatch = require("minimatch"); | ||
|
|
||
| const asyncWithCache = cosmiconfig("prettier"); | ||
| const asyncNoCache = cosmiconfig("prettier", { cache: false }); | ||
| const syncWithCache = cosmiconfig("prettier", { sync: true }); | ||
| const syncNoCache = cosmiconfig("prettier", { cache: false, sync: true }); | ||
|
|
||
| const editorconfigAsyncNoCache = filePath => | ||
| filePath && editorconfig.parse(filePath).then(editorConfigToPrettier); | ||
| const editorconfigAsyncWithCache = mem(editorconfigAsyncNoCache); | ||
| const editorconfigSyncNoCache = filePath => | ||
| filePath && editorConfigToPrettier(editorconfig.parseSync(filePath)); | ||
|
There was a problem hiding this comment. This arrow function has no braces while the above when has. |
||
| const editorconfigSyncWithCache = mem(editorconfigSyncNoCache); | ||
|
|
||
| function editorConfigToPrettier(editorConfig) { | ||
| const result = {}; | ||
|
|
||
| if (editorConfig.indent_style) { | ||
| result.useTabs = editorConfig.indent_style === "tab"; | ||
| } | ||
|
|
||
| const tabWidth = editorConfig.indent_size || editorConfig.tab_width; | ||
| if (tabWidth) { | ||
| result.tabWidth = tabWidth; | ||
| } | ||
|
|
||
| if (editorConfig.max_line_length) { | ||
| result.printWidth = editorConfig.max_line_length; | ||
| } | ||
|
|
||
|
There was a problem hiding this comment. Is it important that the 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 There was a problem hiding this comment.
Yes, I think that's what fixed this test: https://travis-ci.org/prettier/prettier/jobs/272596635#L379
Hmm, it looks like tab_width and max_line_length must be positive. I suppose someone could set |
||
| return result; | ||
| } | ||
|
|
||
| function resolveConfig(filePath, opts) { | ||
| const useCache = !(opts && opts.useCache === false); | ||
| return Promise.all([ | ||
| (useCache ? asyncWithCache : asyncNoCache).load(filePath), | ||
| (useCache ? editorconfigAsyncWithCache : editorconfigAsyncNoCache)(filePath) | ||
| ]).then((arr /* [result, editorConfigged] */) => | ||
| mergeEditorConfig(arr[0], filePath, arr[1]) | ||
|
There was a problem hiding this comment. Can we put |
||
| ); | ||
| } | ||
|
|
||
| resolveConfig.sync = (filePath, opts) => { | ||
| const useCache = !(opts && opts.useCache === false); | ||
| const result = (useCache ? syncWithCache : syncNoCache).load(filePath); | ||
| const editorConfigged = (useCache | ||
| ? editorconfigSyncWithCache | ||
| : editorconfigSyncNoCache)(filePath); | ||
| return mergeEditorConfig(result, filePath, editorConfigged); | ||
| }; | ||
|
|
||
| function mergeEditorConfig(result, filePath, editorConfigged) { | ||
| if (!filePath) { | ||
|
There was a problem hiding this comment.
There was a problem hiding this comment. Yeah, I think I had to add that branch in to keep the pre-existing tests passing. (related to #2997) |
||
| return null; | ||
|
There was a problem hiding this comment. Unrelated: it'd be better to add a comment to describe why we return There was a problem hiding this comment. It turns out we could get rid of this altogether. Fixed in 49a19fe |
||
| } | ||
|
|
||
| const config = result && result.config; | ||
|
|
||
| return Object.assign({}, editorConfigged, mergeOverrides(config, filePath)); | ||
| } | ||
|
|
||
| function clearCache() { | ||
| syncWithCache.clearCaches(); | ||
| asyncWithCache.clearCaches(); | ||
|
|
||
| editorconfigSyncWithCache.clear(); | ||
| editorconfigAsyncWithCache.clear(); | ||
|
There was a problem hiding this comment. It seems we need the There was a problem hiding this comment. Perhaps time to add tests for the clearCache function, then? There was a problem hiding this comment. I tried that when I originally added it but I was having issues with jest module mocks so I gave up. We should look at it again. There was a problem hiding this comment. Oops, I added (and fixed) a test to ensure it doesn't throw. Haven't yet gotten to testing that the cache actually clears. |
||
| } | ||
|
|
||
| function resolveConfigFile(filePath) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| root = true | ||
|
|
||
| [*] | ||
|
There was a problem hiding this comment. We should test a more specific glob as well, with different settings, like |
||
| indent_style = tab | ||
| tab_width = 8 | ||
| max_line_length = 100 | ||
|
|
||
| # Indentation override for all JS under lib directory | ||
| [lib/**.js] | ||
| indent_style = space | ||
| indent_size = 2 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| function f() { | ||
| console.log("should have tab width 8"); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| function f() { | ||
| console.log("should have space width 2"); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is starting to blow up combinatorially. Any way we can apply some polymorphism magic to clean it up a bit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ikatyang I think the "manual" way is better in this case :)