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

Conversation

fisker
Copy link
Member

@fisker fisker commented Feb 10, 2023

Description

Fixes #8048

Checklist

  • I’ve added tests to confirm my change works.
  • (If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory).
  • (If the change is user-facing) I’ve added my changes to changelog_unreleased/*/XXXX.md file following changelog_unreleased/TEMPLATE.md.
  • I’ve read the contributing guidelines.

Try the playground for this PR

@fisker fisker changed the base branch from next to main March 29, 2023 09:48
@fisker fisker marked this pull request as ready for review March 29, 2023 10:37
@fisker
Copy link
Member Author

fisker commented Mar 29, 2023

@prettier/core Please review.

# Conflicts:
#	tests/integration/__tests__/__snapshots__/help-options.js.snap
#	tests/integration/__tests__/__snapshots__/ignore-path.js.snap
Comment on lines +53 to +55
if (ignoreFilePaths.length === 0 && !withNodeModules) {
ignoreFilePaths = [undefined];
}
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)

Copy link
Member

@kachkaev kachkaev left a comment

Choose a reason for hiding this comment

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

I guess we need to tweak type definitions too:

ignorePath?: string | undefined;

  ignorePath?: string | string[] | undefined;

Apart from that, LGTM! The change looks backwards-compatible, so it’d be nice to see it in v3.0 or 3.1!

If anyone else has time to take a look at the diff, please do 🙏

@fisker fisker merged commit 578f093 into prettier:main Apr 10, 2023
29 checks passed
@fisker fisker deleted the multiple-ignore-path branch April 10, 2023 02:51
@lostinpatterns
Copy link

@fisker any chance of cutting a new release now that this is merged?

@kachkaev kachkaev added this to the 3.0 milestone Apr 18, 2023
@kachkaev
Copy link
Member

@lostinpatterns this change will be included into v3.0

@erikpukinskis
Copy link

@lostinpatterns FYI this is in prettier@next, it's just not tagged on Github!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multiple files for --ignore-path
5 participants