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

Update ignore to v5 #8546

Closed
wants to merge 26 commits into from
Closed

Update ignore to v5 #8546

wants to merge 26 commits into from

Conversation

fisker
Copy link
Member

@fisker fisker commented Jun 12, 2020

Previous attempt #6975

  • 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/*/pr-XXXX.md file following changelog_unreleased/TEMPLATE.md.
  • I’ve read the contributing guidelines.

Try the playground for this PR

src/common/get-file-info.js Outdated Show resolved Hide resolved
@fisker fisker marked this pull request as draft June 17, 2020 07:32
# Conflicts:
#	src/common/get-file-info.js
#	yarn.lock
# Conflicts:
#	src/common/get-file-info.js
@fisker fisker marked this pull request as ready for review June 19, 2020 14:31
@thorn0
Copy link
Member

thorn0 commented Jun 23, 2020

I'd like to take a deeper look at this PR, but I need time for that. Please don't merge for now.

# Conflicts:
#	package.json
Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Ready for review?

@fisker
Copy link
Member Author

fisker commented Aug 17, 2020

It's ready long time ago, just solved the conflicts, still waiting for @thorn0 review.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Looks good for me

@thorn0
Copy link
Member

thorn0 commented Aug 23, 2020

Here's what bothers me about this change.

./foo/ignore.txt

*.js

./bar.js

1
prettier --ignore-path foo/ignore.txt .

Result with Prettier 2.0.5: bar.js is ignored.
Result with this PR: bar.js isn't ignored.

So this change is breaking.

@fisker
Copy link
Member Author

fisker commented Aug 24, 2020

@thorn0 ignore@5 works like this, and I think it's reasonable, add I did add a test to confirm that "Should not ignore files in parent dir".


I did edit this comment several times, get little confused by this problem, I think this(original) is right explanation.

@fisker
Copy link
Member Author

fisker commented Aug 24, 2020

FYI, the related file path pass to ignore.ignores is ../bar.js, so ignore didn't ignore files in parent dir. (This change was made in #7588)

@thorn0
Copy link
Member

thorn0 commented Aug 24, 2020

I understand why it works this way, but this is still a breaking change.

@fisker
Copy link
Member Author

fisker commented Aug 24, 2020

If the breaking change is what you worried about, we can merge later.

Most important is, which behavior do you prefer? I think the new one not bad.

@thorn0
Copy link
Member

thorn0 commented Aug 24, 2020

The new behavior is aligned with Git. It looks simpler and cleaner to me, especially if support for multiple ignore files is going to be added. But as long as Prettier doesn't have that support and because we don't know how and why people usually use --ignore-path with subdirectories, I'd prefer to keep the current behavior to stay on the safe side.

In order to both keep the behavior and update ignore, a workaround seems to be possible: a separate ignorer for paths that start with ../. It should use only patterns that don't have a slash at the beginning or middle and patterns that start with **/. It would be safe then to strip .. from the relative paths passed to it.

# Conflicts:
#	package.json
#	src/cli/util.js
#	src/common/get-file-info.js
#	tests_integration/__tests__/__snapshots__/file-info.js.snap
#	tests_integration/__tests__/file-info.js
#	tests_integration/__tests__/ignore-emoji.js
#	yarn.lock
@fisker
Copy link
Member Author

fisker commented Dec 20, 2021

Use ignore@5.2.0 with allowRelativePaths: true options, we don't need this anymore. See #11990

@fisker fisker closed this Dec 20, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants