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

Respect EditorConfig settings #2760

Merged
merged 61 commits into from Nov 8, 2017
Merged
Show file tree
Hide file tree
Changes from 52 commits
Commits
Show all changes
61 commits
Select commit Hold shift + click to select a range
f3a6ecb
[WIP] Respect EditorConfig settings
josephfrazier Sep 3, 2017
6897f6b
Merge branch 'master' into editorconfig
josephfrazier Sep 6, 2017
24821d5
Add test for more specific .editorconfig glob
josephfrazier Sep 6, 2017
9a7a17a
Update config-resolution test snapshots
josephfrazier Sep 6, 2017
7d92baf
Fix lint
josephfrazier Sep 6, 2017
2a1ae88
fixup! Update config-resolution test snapshots
josephfrazier Sep 6, 2017
54a5dcb
Ignore absent EditorConfig settings
josephfrazier Sep 7, 2017
4f52651
Remove unnecessary eslint-disable comments
josephfrazier Sep 7, 2017
ad69825
Merge branch 'master' into editorconfig
josephfrazier Sep 9, 2017
bf534cd
WIP extract helper function
josephfrazier Sep 9, 2017
53e6f17
Move editorconfig parsing outside helper
josephfrazier Sep 12, 2017
4f660e6
use async editorconfig
josephfrazier Sep 12, 2017
ccd03c6
parallelize async resolveConfig
josephfrazier Sep 12, 2017
5e6ef51
Merge branch 'master' into editorconfig
josephfrazier Sep 12, 2017
a42fa75
Add caching for editorconfig
josephfrazier Sep 12, 2017
f123c64
Fix syntax for Node v4
josephfrazier Sep 12, 2017
c4bb231
Honor editorconfig even if no prettier config is found
josephfrazier Sep 12, 2017
8404ee0
Remove unnecessary braces from arrow function
josephfrazier Sep 12, 2017
85e350b
Return null from resolveConfig() when filePath isn't provided
josephfrazier Sep 12, 2017
148d3bf
rename helper function
josephfrazier Sep 12, 2017
cfdb146
Test that prettier.clearConfigCache() doesn't throw
josephfrazier Sep 13, 2017
efed301
Use mem.clear() instead of .clear() method
josephfrazier Sep 13, 2017
5c9c3d9
Put `filePath` in front of `arr[x]`
josephfrazier Sep 13, 2017
f69d367
Make variable names explicit
josephfrazier Sep 13, 2017
e422fc0
Merge branch 'master' into editorconfig
josephfrazier Sep 14, 2017
489fc24
WIP Test that .editorconfig is overridden by other configs
josephfrazier Sep 14, 2017
f859805
Merge branch 'master' into editorconfig
josephfrazier Sep 14, 2017
7c2b545
Disregard .editorconfig when `--config` is passed
josephfrazier Sep 14, 2017
e2bc881
Regenerate README TOC
josephfrazier Sep 14, 2017
a9a8b57
Add EditorConfig info to README
josephfrazier Sep 14, 2017
6efbf89
Pin dependency versions
josephfrazier Sep 14, 2017
0ff7bec
Merge branch 'master' into editorconfig
josephfrazier Oct 8, 2017
cd1e260
Use editorconfig fork with Rollup fix
josephfrazier Oct 8, 2017
59adb59
Always return a Promise from `editorconfigAsyncNoCache`
josephfrazier Oct 8, 2017
6a95dc2
Add getLoadEditorConfigFunction like getLoadFunction
josephfrazier Oct 8, 2017
99b9619
Pass opts.config to loadEditorConfig() like load()
josephfrazier Oct 8, 2017
30c619f
Extract loadOpts
josephfrazier Oct 8, 2017
48abd24
Add sync argument to resolveConfig, use in resolveConfig.sync
josephfrazier Oct 8, 2017
f07c166
Move EditorConfig stuff into own file
josephfrazier Oct 8, 2017
619ae2e
Remove unnecessary `.js` from `require()`
josephfrazier Oct 9, 2017
f5f382b
Don't expose `sync` parameter of resolveConfig
josephfrazier Oct 9, 2017
e583cc1
Simplify resolveConfig.sync definition
josephfrazier Oct 9, 2017
734d5ad
Switch back to upstream editorconfig
josephfrazier Oct 9, 2017
3fef30d
Merge branch 'master' into editorconfig
josephfrazier Oct 12, 2017
ccc3b01
Clarify variable name: editorConfigged -> editorConfigured
josephfrazier Oct 12, 2017
8776fa6
Pass path root to editorconfig
josephfrazier Oct 12, 2017
e5e8da0
Merge branch 'master' into editorconfig
josephfrazier Oct 15, 2017
1b4b1b4
mergeEditorConfig: Ensure mergeOverrides argument is defined
josephfrazier Oct 15, 2017
128e6e8
Dedupe conditional editorconfig parsing
josephfrazier Oct 15, 2017
e9c22f9
Make test .editorconfig not set config for all file extensions
josephfrazier Oct 17, 2017
6ac1ba7
Make test .prettierrc not set config for all file extensions
josephfrazier Oct 17, 2017
49a19fe
Return null from resolveConfig when no config is found
josephfrazier Oct 17, 2017
d44420c
Clarify when resolveConfig returns null
josephfrazier Oct 18, 2017
db9877c
editorconfig: test indent_style = tab overriding indent_size with tab…
josephfrazier Nov 2, 2017
a9bc09e
editorconfig: prioritize tab_width over indent_size
josephfrazier Nov 1, 2017
26f49a4
editorconfig: let indent_style force either tab_width or indent_size …
josephfrazier Nov 1, 2017
bf282f9
editorconfig: test/fix indent_size = tab
josephfrazier Nov 2, 2017
27254d4
Move editorConfigToPrettier into separate package
josephfrazier Nov 2, 2017
5526de0
Merge branch 'master' into editorconfig
josephfrazier Nov 6, 2017
664dced
Merge branch 'master' into editorconfig
josephfrazier Nov 6, 2017
47e4b62
Merge branch 'master' into josephfrazier-editorconfig
josephfrazier Nov 8, 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
9 changes: 9 additions & 0 deletions README.md
Expand Up @@ -57,6 +57,7 @@ conforms to a consistent style. (See this [blog post](http://jlongster.com/A-Pre
+ [Basic Configuration](#basic-configuration)
+ [Configuration Overrides](#configuration-overrides)
+ [Configuration Schema](#configuration-schema)
+ [EditorConfig](#editorconfig)
* [Editor Integration](#editor-integration)
+ [Atom](#atom)
+ [Emacs](#emacs)
Expand Down Expand Up @@ -788,6 +789,14 @@ For more information on how to use the CLI to locate a file, see the [CLI](#cli)

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

### EditorConfig

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`

## Editor Integration

### Atom
Expand Down
2 changes: 2 additions & 0 deletions package.json
Expand Up @@ -22,6 +22,7 @@
"cosmiconfig": "3.1.0",
"dashify": "0.2.2",
"diff": "3.2.0",
"editorconfig": "0.14.2",
"emoji-regex": "6.5.1",
"escape-string-regexp": "1.0.5",
"esutils": "2.0.2",
Expand All @@ -37,6 +38,7 @@
"minimatch": "3.0.4",
"minimist": "1.2.0",
"parse5": "3.0.2",
"path-root": "0.1.1",
"postcss-less": "1.1.1",
"postcss-media-query-parser": "0.2.3",
"postcss-scss": "1.0.0",
Expand Down
62 changes: 62 additions & 0 deletions src/resolve-config-editorconfig.js
@@ -0,0 +1,62 @@
"use strict";

const editorconfig = require("editorconfig");
const mem = require("mem");
const pathRoot = require("path-root");

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);
}

function editorConfigToPrettier(editorConfig) {
editorConfig = editorConfig || {};
const result = {};

if (editorConfig.indent_style) {
result.useTabs = editorConfig.indent_style === "tab";
}

const tabWidth = editorConfig.indent_size || editorConfig.tab_width;
Copy link

Choose a reason for hiding this comment

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

It's an edge case, but technically I believe it's possible to define both an indent_size and a tab_width, where the latter should be preferred in case indent_style is tab. In fact, indent_size can actually be set to tab. This line would, therefore, get the wrong prettier tabWidth or break completely?

https://github.com/prettier/prettier-atom/blob/master/src/executePrettier/buildEditorConfigOptions.test.js#L15-L21

if (tabWidth) {
result.tabWidth = tabWidth;
}

if (editorConfig.max_line_length) {
result.printWidth = editorConfig.max_line_length;
}

return result;
}

module.exports = {
getLoadFunction,
clearCache
};
44 changes: 33 additions & 11 deletions src/resolve-config.js
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 @@ -20,23 +22,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) {
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.

I'm not sure if we should treat 0-option as not-found, but if so, we should document it on the README as well.

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'm thinking that we shouldn't distinguish between the cases where .prettierrc/.editorconfig are not present, and the cases where they are but none of their configurations pertain to the file in question. Given that the goal of resolveConfig is to find the configuration for a specific file, it doesn't seem especially useful to return an empty object.

I just clarified this in d44420c

Copy link
Member

@CiGit CiGit Oct 18, 2017

Choose a reason for hiding this comment

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

I was thinking in using this difference to test for the existence of a .prettierrc to implement format only when there is a config (vscode).

  • Empty bag -> use defaults
  • null -> skip

@robwise May have implemented it this way (atom).

Anyway, if it also takes editorConfig into account, this is not what I would need anymore. Would it be possible to expose findConfigPath in that case?

Copy link
Member

Choose a reason for hiding this comment

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

Yes we definitely want to keep this distinction. An empty config file is different to no config file.

Copy link

Choose a reason for hiding this comment

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

@robwise May have implemented it this way (atom).

Right now I can't tell the difference between an empty config file and a missing one, so I have to put a special note to users that they need to have a rule in it in order for it to be picked up:

prettier config format on save options

It would be nice to have a way to differentiate so we won't have this edge case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the feedback, everyone! I see why being able to differentiate between these cases is useful, so I'll try to add some patches implementing/testing this.

EDIT: Oh, @CiGit:

Anyway, if it also takes editorConfig into account, this is not what I would need anymore. Would it be possible to expose findConfigPath in that case?

I assume you're talking about resolveConfigFile when you say findConfigPath. Anyway, yeah, it occurred to me that taking .editorconfig into account makes it impossible for any one file path to be the correct result of resolveConfigFile. From what I can tell, resolveConfigFile is intended mainly to be used by editor integrations that repeatedly call prettier via the CLI. If this is the case, I'm hoping that either/both:

  • Developers who use these integrations will port the .editorconfig settings to .prettierrc
  • The integrations can be rewritten to use Prettier's API

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see why being able to differentiate between these cases is useful, so I'll try to add some patches implementing/testing this

I thought about this a bit more, and I'd actually like to let it be part of a separate PR later, since it's a pre-existing issue. That way, this PR can focus on the editorconfig functionality.

}

return merged;
};

if (loadOpts.sync) {
return unwrapAndMerge(arr);
}

Copy link
Member

@lydell lydell Sep 7, 2017

Choose a reason for hiding this comment

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

Is it important that the .useTabs, .tabWidth and .printWidth properties of result are missing rather than present with the value undefined if the corresponding EditorConfig setting is missing?

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 indent_size, tab_width and max_line_length to 0, and as such we should respect that? Note that 0 if a falsy value.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is it important that the .useTabs, .tabWidth and .printWidth properties of result are missing rather than present with the value undefined if the corresponding EditorConfig setting is missing?

Yes, I think that's what fixed this test: https://travis-ci.org/prettier/prettier/jobs/272596635#L379

While being an odd configuration, isn't it valid to set indent_size, tab_width and max_line_length to 0, and as such we should respect that? Note that 0 if a falsy value.

Hmm, it looks like tab_width and max_line_length must be positive. I suppose someone could set indent_size to 0, but I'm not sure if that edge case is worth supporting right now. At least, I haven't had time to put effort into it.

Copy link
Member

Choose a reason for hiding this comment

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

I guess it's all fine then 👍

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
Expand Up @@ -3,7 +3,17 @@
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\\"
)
}
console.log(
\\"jest/__best-tests__/file.js should have semi\\"
);
console.log(
Expand Down Expand Up @@ -84,7 +94,13 @@ 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\\")
}
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
Expand Up @@ -80,7 +80,17 @@ 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\\"
)
}
console.log(
\\"jest/__best-tests__/file.js should have semi\\"
);
console.log(
Expand Down Expand Up @@ -137,7 +147,17 @@ 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\\"
)
}
console.log(
\\"jest/__best-tests__/file.js should have semi\\"
);
console.log(
Expand Down
68 changes: 68 additions & 0 deletions tests_integration/__tests__/config-resolution.js
Expand Up @@ -102,6 +102,74 @@ 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 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();
Copy link
Member

Choose a reason for hiding this comment

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

This is somewhat out of scope for this PR, but it's an interesting discussion: Are we happy with this test, or do we want to do something like:

  1. Read config with cache.
  2. Change config file on disk.
  3. Read with cache again. Still reports the same as in step 1.
  4. Clear cache.
  5. Read again. Now the changes from step 2 are picked up.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That sounds pretty good to me. We could add another step before 3 (changes in bold)

  1. Read config with cache.
  2. Change config file on disk.
  3. Read without cache. Now the changes from step 2 are picked up.
  4. Read with cache again. Still reports the same as in step 1.
  5. Clear cache.
  6. Read with cache again. Now the changes from step 2 are picked up.

I'd like to do this in a separate PR, though, at least for the non-editorconfig caches (and maybe the editorconfig caches too)

});

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
@@ -1,6 +1,7 @@
semi: false

overrides:
- files: "*.js"
options:
semi: false
- files: "*.ts"
options:
semi: true
11 changes: 11 additions & 0 deletions tests_integration/cli/config/editorconfig/.editorconfig
@@ -0,0 +1,11 @@
root = true

[*.js]
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
3 changes: 3 additions & 0 deletions tests_integration/cli/config/editorconfig/file.js
@@ -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
@@ -0,0 +1,3 @@
function f() {
console.log("should have space width 2");
}
4 changes: 4 additions & 0 deletions tests_integration/cli/config/js/.editorconfig
@@ -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
@@ -0,0 +1,7 @@
# This file should be overridden by package.json

[*]
tab_width = 1

[*.ts]
tab_width = 1