-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Re-add EditorConfig support (undo #3213) #3255
Changes from 3 commits
9892602
ddfa529
6dbd67f
b2637d7
2f643e9
aac81c4
d0beb79
bcf6f03
f9c8cc3
d5dcbfa
7597be5
89bf8bd
64a4413
1ba5e6c
53013cb
a36f9b9
e3460b8
a2c81fc
0fda128
f74aa23
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
"use strict"; | ||
|
||
const editorconfig = require("editorconfig"); | ||
const mem = require("mem"); | ||
const pathRoot = require("path-root"); | ||
const editorConfigToPrettier = require("editorconfig-to-prettier"); | ||
|
||
const maybeParse = (filePath, config, parse) => { | ||
const root = filePath && pathRoot(filePath); | ||
return filePath && !config && parse(filePath, { root }); | ||
}; | ||
|
||
const editorconfigAsyncNoCache = (filePath, config) => { | ||
return Promise.resolve(maybeParse(filePath, config, editorconfig.parse)).then( | ||
editorConfigToPrettier | ||
); | ||
}; | ||
const editorconfigAsyncWithCache = mem(editorconfigAsyncNoCache); | ||
|
||
const editorconfigSyncNoCache = (filePath, config) => { | ||
return editorConfigToPrettier( | ||
maybeParse(filePath, config, editorconfig.parseSync) | ||
); | ||
}; | ||
const editorconfigSyncWithCache = mem(editorconfigSyncNoCache); | ||
|
||
function getLoadFunction(opts) { | ||
if (opts.sync) { | ||
return opts.cache ? editorconfigSyncWithCache : editorconfigSyncNoCache; | ||
} | ||
|
||
return opts.cache ? editorconfigAsyncWithCache : editorconfigAsyncNoCache; | ||
} | ||
|
||
function clearCache() { | ||
mem.clear(editorconfigSyncWithCache); | ||
mem.clear(editorconfigAsyncWithCache); | ||
} | ||
|
||
module.exports = { | ||
getLoadFunction, | ||
clearCache | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,8 @@ const minimatch = require("minimatch"); | |
const path = require("path"); | ||
const mem = require("mem"); | ||
|
||
const resolveEditorConfig = require("./resolve-config-editorconfig"); | ||
|
||
const getExplorerMemoized = mem(opts => | ||
cosmiconfig("prettier", { | ||
sync: opts.sync, | ||
|
@@ -26,23 +28,43 @@ function getLoadFunction(opts) { | |
return getExplorerMemoized(opts).load; | ||
} | ||
|
||
function resolveConfig(filePath, opts) { | ||
function _resolveConfig(filePath, opts, sync) { | ||
opts = Object.assign({ useCache: true }, opts); | ||
const load = getLoadFunction({ cache: !!opts.useCache, sync: false }); | ||
return load(filePath, opts.config).then(result => { | ||
return !result ? null : mergeOverrides(result, filePath); | ||
}); | ||
const loadOpts = { cache: !!opts.useCache, sync: !!sync }; | ||
const load = getLoadFunction(loadOpts); | ||
const loadEditorConfig = resolveEditorConfig.getLoadFunction(loadOpts); | ||
const arr = [load, loadEditorConfig].map(l => l(filePath, opts.config)); | ||
|
||
const unwrapAndMerge = arr => { | ||
const result = arr[0]; | ||
const editorConfigured = arr[1]; | ||
const merged = Object.assign( | ||
{}, | ||
editorConfigured, | ||
mergeOverrides(Object.assign({}, result), filePath) | ||
); | ||
|
||
if (Object.keys(merged).length === 0) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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:
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))
Hmm, I guess the use-case here is:
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Ok, so one way to fix this is to make it so that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Hmm, it looks like cosmiconfig intentionally ignores empty files: cosmiconfig/cosmiconfig@1adf5c2 However, a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this be: if (!result && !editorConfigured) {
return null;
} ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (e.g. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch, fixed in e3460b8! This change exposed a bug in some of the tests added in this PR, so I had to remove them. Note that the bug in question would also affect a similar test added on master, so I think this is ok for now. See a2c81fc for details, and #2760 (comment) for previous discussion of the no-longer-added tests. |
||
return null; | ||
} | ||
|
||
return merged; | ||
}; | ||
|
||
if (loadOpts.sync) { | ||
return unwrapAndMerge(arr); | ||
} | ||
|
||
return Promise.all(arr).then(unwrapAndMerge); | ||
} | ||
|
||
resolveConfig.sync = (filePath, opts) => { | ||
opts = Object.assign({ useCache: true }, opts); | ||
const load = getLoadFunction({ cache: !!opts.useCache, sync: true }); | ||
const result = load(filePath, opts.config); | ||
return !result ? null : mergeOverrides(result, filePath); | ||
}; | ||
const resolveConfig = (filePath, opts) => _resolveConfig(filePath, opts, false); | ||
|
||
resolveConfig.sync = (filePath, opts) => _resolveConfig(filePath, opts, true); | ||
|
||
function clearCache() { | ||
mem.clear(getExplorerMemoized); | ||
resolveEditorConfig.clearCache(); | ||
} | ||
|
||
function resolveConfigFile(filePath) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
semi: false | ||
|
||
overrides: | ||
- files: "*.js" | ||
options: | ||
semi: false | ||
- files: "*.ts" | ||
options: | ||
semi: true |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
root = true | ||
|
||
[*.js] | ||
indent_style = tab | ||
tab_width = 8 | ||
indent_size = 2 # overridden by tab_width since indent_style = tab | ||
max_line_length = 100 | ||
|
||
# Indentation override for all JS under lib directory | ||
[lib/**.js] | ||
indent_style = space | ||
indent_size = 2 | ||
|
||
[lib/indent_size=tab.js] | ||
indent_size = tab |
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"); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
function f() { | ||
console.log("should have space width 8"); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
# This file should be overridden by prettier.config.js | ||
|
||
[*] | ||
tab_width = 1 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
# This file should be overridden by package.json | ||
|
||
[*] | ||
tab_width = 1 | ||
|
||
[*.ts] | ||
tab_width = 1 |
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.
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.