Skip to content
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

Merged
merged 20 commits into from
Dec 4, 2017
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
9892602
Revert "Revert "Respect EditorConfig settings" (#3213)"
josephfrazier Nov 12, 2017
ddfa529
Comment out EditorConfig docs
josephfrazier Nov 12, 2017
6dbd67f
editorconfig: Support `indent_size = 0`
josephfrazier Nov 12, 2017
b2637d7
Revert "Comment out EditorConfig docs"
josephfrazier Nov 13, 2017
2f643e9
Mark EditorConfig functionality as v1.9.0+
josephfrazier Nov 13, 2017
aac81c4
Merge branch 'master' into editorconfig-2
josephfrazier Nov 22, 2017
d0beb79
editorconfig: Upgrade editorconfig-to-prettier to 0.0.4
josephfrazier Nov 27, 2017
bcf6f03
editorconfig: Only enable for CLI, by default
josephfrazier Dec 3, 2017
f9c8cc3
editorconfig: Add tests confirming that editorconfig is ignored by de…
josephfrazier Dec 3, 2017
d5dcbfa
Merge branch 'master' into editorconfig-2
josephfrazier Dec 3, 2017
7597be5
editorconfig: Add/fix CLI option parsing
josephfrazier Dec 3, 2017
89bf8bd
editorconfig: Move docs from configuration.md to options.md
josephfrazier Dec 3, 2017
64a4413
editorconfig: Add `oppositeDescription` to show docs for `--no-editor…
josephfrazier Dec 4, 2017
1ba5e6c
editorconfig: Update test snapshots
josephfrazier Dec 4, 2017
53013cb
editorconfig: Remove unnecessary options parsing code
josephfrazier Dec 4, 2017
a36f9b9
editorconfig: Move docs from options.md to api.md and cli.md
josephfrazier Dec 4, 2017
e3460b8
resolveConfig: return null if both .prettierrc and .editorconfig are …
josephfrazier Dec 4, 2017
a2c81fc
Don't add now-failing tests
josephfrazier Dec 4, 2017
0fda128
Merge branch 'master' into editorconfig-2
azz Dec 4, 2017
f74aa23
Merge branch 'master' into editorconfig-2
azz Dec 4, 2017
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 10 additions & 0 deletions docs/configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

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.


If an [`.editorconfig` file](http://editorconfig.org/) is in your project, Prettier will parse it and convert its properties to the corresponding prettier configuration. This configuration will be overridden by `.prettierrc`, etc. Currently, the following EditorConfig properties are supported:

* `indent_style`
* `indent_size`/`tab_width`
* `max_line_length`
-->
3 changes: 3 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
"cosmiconfig": "3.1.0",
"dashify": "0.2.2",
"diff": "3.2.0",
"editorconfig": "0.14.2",
"editorconfig-to-prettier": "0.0.2",
"emoji-regex": "6.5.1",
"escape-string-regexp": "1.0.5",
"esutils": "2.0.2",
Expand All @@ -37,6 +39,7 @@
"minimatch": "3.0.4",
"minimist": "1.2.0",
"parse5": "3.0.3",
"path-root": "0.1.1",
"postcss-less": "1.1.1",
"postcss-media-query-parser": "0.2.3",
"postcss-scss": "1.0.2",
Expand Down
43 changes: 43 additions & 0 deletions src/resolve-config-editorconfig.js
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
};
44 changes: 33 additions & 11 deletions src/resolve-config.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The 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 .editorconfig... I don't know. Editors want to support formatting only if a config file is found, so supporting EditorConfig might break that functionality.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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 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):

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be:

if (!result && !editorConfigured) {
  return null;
}

?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(e.g. .prettierrc file containing {} should return {}, not null)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,22 @@
exports[`CLI overrides take precedence (stderr) 1`] = `""`;

exports[`CLI overrides take precedence (stdout) 1`] = `
"console.log(
"function f() {
console.log(
\\"should have tab width 8\\"
)
}
function f() {
console.log(
\\"should have space width 2\\"
)
}
function f() {
console.log(
\\"should have space width 8\\"
)
}
console.log(
\\"jest/__best-tests__/file.js should have semi\\"
);
console.log(
Expand Down Expand Up @@ -84,7 +99,16 @@ exports[`resolves configuration file with --find-config-path file (write) 1`] =
exports[`resolves configuration from external files (stderr) 1`] = `""`;

exports[`resolves configuration from external files (stdout) 1`] = `
"console.log(\\"jest/__best-tests__/file.js should have semi\\");
"function f() {
console.log(\\"should have tab width 8\\")
}
function f() {
console.log(\\"should have space width 2\\")
}
function f() {
console.log(\\"should have space width 8\\")
}
console.log(\\"jest/__best-tests__/file.js should have semi\\");
console.log(\\"jest/Component.js should not have semi\\")
console.log(\\"jest/Component.test.js should have semi\\");
function js() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,22 @@ exports[`CLI overrides take lower precedence with --config-precedence file-overr
exports[`CLI overrides take precedence with --config-precedence cli-override (stderr) 1`] = `""`;

exports[`CLI overrides take precedence with --config-precedence cli-override (stdout) 1`] = `
"console.log(
"function f() {
console.log(
\\"should have tab width 8\\"
)
}
function f() {
console.log(
\\"should have space width 2\\"
)
}
function f() {
console.log(
\\"should have space width 8\\"
)
}
console.log(
\\"jest/__best-tests__/file.js should have semi\\"
);
console.log(
Expand Down Expand Up @@ -137,7 +152,22 @@ exports[`CLI overrides take precedence with --config-precedence cli-override (wr
exports[`CLI overrides take precedence without --config-precedence (stderr) 1`] = `""`;

exports[`CLI overrides take precedence without --config-precedence (stdout) 1`] = `
"console.log(
"function f() {
console.log(
\\"should have tab width 8\\"
)
}
function f() {
console.log(
\\"should have space width 2\\"
)
}
function f() {
console.log(
\\"should have space width 8\\"
)
}
console.log(
\\"jest/__best-tests__/file.js should have semi\\"
);
console.log(
Expand Down
92 changes: 92 additions & 0 deletions tests_integration/__tests__/config-resolution.js
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,98 @@ test("API resolveConfig.sync with file arg and extension override", () => {
});
});

test("API resolveConfig with file arg and .editorconfig", () => {
const file = path.resolve(
path.join(__dirname, "../cli/config/editorconfig/file.js")
);
return prettier.resolveConfig(file).then(result => {
expect(result).toMatchObject({
useTabs: true,
tabWidth: 8,
printWidth: 100
});
});
});

test("API resolveConfig.sync with file arg and .editorconfig", () => {
const file = path.resolve(
path.join(__dirname, "../cli/config/editorconfig/file.js")
);
expect(prettier.resolveConfig.sync(file)).toMatchObject({
useTabs: true,
tabWidth: 8,
printWidth: 100
});
});

test("API resolveConfig with nested file arg and .editorconfig", () => {
const file = path.resolve(
path.join(__dirname, "../cli/config/editorconfig/lib/file.js")
);
return prettier.resolveConfig(file).then(result => {
expect(result).toMatchObject({
useTabs: false,
tabWidth: 2,
printWidth: 100
});
});
});

test("API resolveConfig.sync with nested file arg and .editorconfig", () => {
const file = path.resolve(
path.join(__dirname, "../cli/config/editorconfig/lib/file.js")
);
expect(prettier.resolveConfig.sync(file)).toMatchObject({
useTabs: false,
tabWidth: 2,
printWidth: 100
});
});

test("API resolveConfig with nested file arg and .editorconfig and indent_size = tab", () => {
const file = path.resolve(
path.join(__dirname, "../cli/config/editorconfig/lib/indent_size=tab.js")
);
return prettier.resolveConfig(file).then(result => {
expect(result).toMatchObject({
useTabs: false,
tabWidth: 8,
printWidth: 100
});
});
});

test("API resolveConfig.sync with nested file arg and .editorconfig and indent_size = tab", () => {
const file = path.resolve(
path.join(__dirname, "../cli/config/editorconfig/lib/indent_size=tab.js")
);
expect(prettier.resolveConfig.sync(file)).toMatchObject({
useTabs: false,
tabWidth: 8,
printWidth: 100
});
});

test("API resolveConfig with missing file arg", () => {
const file = path.resolve(
path.join(__dirname, "../cli/config/editorconfig/file.shouldnotexist")
);
return prettier.resolveConfig(file).then(result => {
expect(result).toBeNull();
});
});

test("API resolveConfig.sync with missing file arg", () => {
const file = path.resolve(
path.join(__dirname, "../cli/config/editorconfig/file.shouldnotexist")
);
expect(prettier.resolveConfig.sync(file)).toBeNull();
});

test("API clearConfigCache", () => {
expect(() => prettier.clearConfigCache()).not.toThrowError();
});

test("API resolveConfig.sync overrides work with absolute paths", () => {
// Absolute path
const file = path.join(__dirname, "../cli/config/filepath/subfolder/file.js");
Expand Down
5 changes: 3 additions & 2 deletions tests_integration/cli/config/.prettierrc
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
15 changes: 15 additions & 0 deletions tests_integration/cli/config/editorconfig/.editorconfig
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
3 changes: 3 additions & 0 deletions tests_integration/cli/config/editorconfig/file.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
function f() {
console.log("should have tab width 8");
}
3 changes: 3 additions & 0 deletions tests_integration/cli/config/editorconfig/lib/file.js
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");
}
4 changes: 4 additions & 0 deletions tests_integration/cli/config/js/.editorconfig
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
7 changes: 7 additions & 0 deletions tests_integration/cli/config/package/.editorconfig
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