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

Accept multiple --ignore-paths #14332

Merged
merged 20 commits into from
Apr 10, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 7 additions & 0 deletions changelog_unreleased/cli/14332.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
#### Accept multiple `--ignore-path` (#14332 by @fisker)

You can now pass multiple `--ignore-path`.

```console
prettier . --ignore-path=.prettier-ignore --ignore-path=.eslintignore
```
2 changes: 1 addition & 1 deletion docs/api.md
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ When Prettier loads configuration files and plugins, the file system structure i

The promise will be rejected if the type of `filePath` is not `string`.

Setting `options.ignorePath` (`string`) and `options.withNodeModules` (`boolean`) influence the value of `ignored` (`false` by default).
Setting `options.ignorePath` (`string | string[]`) and `options.withNodeModules` (`boolean`) influence the value of `ignored` (`false` by default).

If the given `filePath` is ignored, the `inferredParser` is always `null`.

Expand Down
3 changes: 2 additions & 1 deletion docs/cli.md
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,8 @@ If you don’t have a configuration file, or want to ignore it if it does exist,

## `--ignore-path`

Path to a file containing patterns that describe files to ignore. By default, Prettier looks for `./.prettierignore`.
Path to a file containing patterns that describe files to ignore. By default, Prettier looks for `./.prettierignore`.\
Multiple values are accepted.

## `--list-different`

Expand Down
8 changes: 6 additions & 2 deletions src/cli/cli-options.evaluate.js
Original file line number Diff line number Diff line change
Expand Up @@ -193,9 +193,13 @@ const options = {
type: "flag",
},
ignorePath: {
array: true,
category: optionCategories.CATEGORY_CONFIG,
default: ".prettierignore",
description: "Path to a file with patterns describing files to ignore.",
default: [{ value: [".prettierignore"] }],
description: outdent`
Path to a file with patterns describing files to ignore.
Multiple values are accepted.
`,
type: "path",
},
ignoreUnknown: {
Expand Down
8 changes: 7 additions & 1 deletion src/common/get-file-info.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,13 @@ async function getFileInfo(filePath, options) {
);
}

const ignored = await isIgnored(filePath, options);
let { ignorePath, withNodeModules } = options;
// In API we allow single `ignorePath`
if (!Array.isArray(ignorePath)) {
ignorePath = [ignorePath];
}

const ignored = await isIgnored(filePath, { ignorePath, withNodeModules });

let inferredParser;
if (!ignored) {
Expand Down
2 changes: 1 addition & 1 deletion src/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -785,7 +785,7 @@ export interface SupportInfo {
}

export interface FileInfoOptions {
ignorePath?: string | undefined;
ignorePath?: string | string[] | undefined;
withNodeModules?: boolean | undefined;
plugins?: string[] | undefined;
resolveConfig?: boolean | undefined;
Expand Down
31 changes: 27 additions & 4 deletions src/utils/ignore.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
* @param {boolean?} withNodeModules
* @returns {Promise<(string) => boolean>}
*/
async function createIsIgnoredFunction(ignoreFilePath, withNodeModules) {
async function createSingleIsIgnoredFunction(ignoreFilePath, withNodeModules) {
let content = "";

if (ignoreFilePath) {
Expand All @@ -25,7 +25,7 @@
}

if (!content) {
return () => false;
return;
}

const ignore = createIgnore({ allowRelativePaths: true }).add(content);
Expand All @@ -44,8 +44,31 @@
}

/**
* @param {string} filepath
* @param {{ignorePath?: string, withNodeModules?: boolean}} options
* @param {string[]} ignoreFilePaths
* @param {boolean?} withNodeModules
* @returns {Promise<(string) => boolean>}
*/
async function createIsIgnoredFunction(ignoreFilePaths, withNodeModules) {
// If `ignoreFilePaths` is empty, we still want `withNodeModules` to work
if (ignoreFilePaths.length === 0 && !withNodeModules) {
ignoreFilePaths = [undefined];
}

Check warning on line 55 in src/utils/ignore.js

View check run for this annotation

Codecov / codecov/patch

src/utils/ignore.js#L54-L55

Added lines #L54 - L55 were not covered by tests
Comment on lines +53 to +55
Copy link
Member

@kachkaev kachkaev Apr 3, 2023

Choose a reason for hiding this comment

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

What if we first read all files in a loop, combine them into a sing big string and then call createIgnore({ allowRelativePaths: true }).add(combinedContent)? That's how I was imagining multiple ignore paths to work from a user perspective. Combining file contents can simplify how withNodeModules is implemented: it will be just a new line appended to combinedContent, so need for introducing [undefined] etc.

Here is an example setup setup where a combined approach kinda matters: 🐸 njt / .prettierignore. This file has three sections, the first of which is specific to Prettier and the other two are copy-pasted from other files. Keeping these secions in sync between files is painful but necessary right now.

With this PR in, I can imagine removing sections named Same as in .gitignore and Shared between linters. To do that, I will be able to add this line to .prettierrc.cjs: ignorePath: [".prettierignore", ".gitignore", ".sharedlintignore"]. That will be great actually!

If we have several independent createIgnore calls plus isIgnoredFunctions.some(...), the overall behavior of the ignorer may differ from what we'd get by ‘concatenating‘ ignore files. This may mismatch user expectations and lead to confusion.

Having said that, I’m not an expert is how ignore files are usually combined. If what you have implemented is common for other linters, I might be the only person who would be confused by the output of isIgnoredFunctions.some(...), compared to having a single createIgnore.

Copy link
Member Author

@fisker fisker Apr 3, 2023

Choose a reason for hiding this comment

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

1, they can be in different directories, they can't be simply concated. We need to know what the patterns related to.

#.ignore1
foo

#dir/.ignore2
bar
  1. In this case
#.ignore1
foo/a.js

#.ignore2
!foo/*
foo/b.js

should foo/a.js be ignored? I think user may expect it be ignored, because it's ignored in .ignore1 file. But if we treat this case like

#.ignore
foo/a.js
!foo/*
foo/b.js

I'm not sure what's expected.

Copy link
Member

@kachkaev kachkaev Apr 3, 2023

Choose a reason for hiding this comment

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

Oh I see... I forgot about ignore files sitting in different directories 🤔 You are right then, we need multiple calls to createIgnore! In that case, the full list of ignored files is a union of ignore statements in individual files. Makes sense now, ignore me 😅

Copy link

Choose a reason for hiding this comment

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

@fisker sorry to ask so late ; it is not told in the docs if the multiple ignore paths would respect their own root folder. for example, how would the final ignore list look like for prettier --ignore-path .ignore1,subfolder/.ignore2 --check subfolder/? (usecase is: running prettier at root level of a monorepo)


const isIgnoredFunctions = (
await Promise.all(
ignoreFilePaths.map((ignoreFilePath) =>
createSingleIsIgnoredFunction(ignoreFilePath, withNodeModules)
)
)
).filter(Boolean);

return (filepath) =>
isIgnoredFunctions.some((isIgnored) => isIgnored(filepath));
}

/**
* @param {string[]} filepath
* @param {{ignorePath: string[], withNodeModules?: boolean}} options
* @returns {Promise<boolean>}
*/
async function isIgnored(filepath, options) {
Expand Down
6 changes: 4 additions & 2 deletions tests/integration/__tests__/__snapshots__/early-exit.js.snap
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,8 @@ Config options:
--find-config-path <path>
Find and print the path to a configuration file for the given input file.
--ignore-path <path> Path to a file with patterns describing files to ignore.
Defaults to .prettierignore.
Multiple values are accepted.
Defaults to [.prettierignore].
--plugin <path> Add a plugin. Multiple plugins can be passed as separate \`--plugin\`s.
Defaults to [].
--plugin-search-dir <path>
Expand Down Expand Up @@ -281,7 +282,8 @@ Config options:
--find-config-path <path>
Find and print the path to a configuration file for the given input file.
--ignore-path <path> Path to a file with patterns describing files to ignore.
Defaults to .prettierignore.
Multiple values are accepted.
Defaults to [.prettierignore].
--plugin <path> Add a plugin. Multiple plugins can be passed as separate \`--plugin\`s.
Defaults to [].
--plugin-search-dir <path>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -249,8 +249,9 @@ exports[`show detailed usage with --help ignore-path (stdout) 1`] = `
"--ignore-path <path>

Path to a file with patterns describing files to ignore.
Multiple values are accepted.

Default: .prettierignore"
Default: [.prettierignore]"
`;

exports[`show detailed usage with --help ignore-path (write) 1`] = `[]`;
Expand Down
16 changes: 0 additions & 16 deletions tests/integration/__tests__/__snapshots__/ignore-path.js.snap
Original file line number Diff line number Diff line change
@@ -1,25 +1,9 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`ignore file when using --debug-check (stderr) 1`] = `""`;

exports[`ignore file when using --debug-check (stdout) 1`] = `"other-regular-modules.js"`;

exports[`ignore file when using --debug-check (write) 1`] = `[]`;

exports[`ignore path (stderr) 1`] = `""`;

exports[`ignore path (stdout) 1`] = `"regular-module.js"`;

exports[`ignore path (write) 1`] = `[]`;

exports[`outputs files as-is if no --write (stderr) 1`] = `""`;

exports[`outputs files as-is if no --write (stdout) 1`] = `"'use strict';"`;

exports[`outputs files as-is if no --write (write) 1`] = `[]`;

exports[`support .prettierignore (stderr) 1`] = `""`;

exports[`support .prettierignore (stdout) 1`] = `"other-regular-modules.js"`;

exports[`support .prettierignore (write) 1`] = `[]`;
26 changes: 26 additions & 0 deletions tests/integration/__tests__/ignore-path.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,24 @@ describe("ignore path", () => {
"-l",
]).test({
status: 1,
stderr: "",
write: [],
});
});

describe("support .prettierignore", () => {
runPrettier("cli/ignore-path", ["**/*.js", "-l"]).test({
status: 1,
stderr: "",
write: [],
});
});

describe("ignore file when using --debug-check", () => {
runPrettier("cli/ignore-path", ["**/*.js", "--debug-check"]).test({
status: 0,
stderr: "",
write: [],
});
});

Expand All @@ -37,5 +43,25 @@ describe("outputs files as-is if no --write", () => {
ignoreLineEndings: true,
}).test({
status: 0,
stderr: "",
write: [],
});
});

describe("multiple `--ignore-path`", () => {
runPrettier("cli/ignore-path", [
"**/*.js",
"--debug-check",
"--ignore-path",
".gitignore",
"--ignore-path",
".prettierignore",
"--ignore-path",
".non-exists-ignore-file",
]).test({
status: 0,
stdout: "",
stderr: "",
write: [],
});
});