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

Allow skipping symlink patterns, to avoid raising a fault #15533

Conversation

sanmai-NL
Copy link
Contributor

@sanmai-NL sanmai-NL commented Oct 23, 2023

Description

In #14627, Prettier was redesigned to ‘no longer follow symbolic links’. However, the implementation went further and turned into a breaking change. Rather than adhering to the exit status contract of only returning a non-zero exit status when a file is not formatted or when something is wrong with Prettier itself, #14627 made Prettier return a non-zero exit status upon any pattern turning out to be a pointing to symlink.

This PR fixes the original PR by @andersk to behave in line with its documentation.

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

@sanmai-NL
Copy link
Contributor Author

@fisker Can you approve to kick off CI?

@fisker
Copy link
Sponsor Member

fisker commented Nov 3, 2023

I think the behavior is expected, but maybe we can skip it if --no-error-on-unmatched-pattern is used.

@sanmai-NL
Copy link
Contributor Author

sanmai-NL commented Nov 3, 2023

@fisker, Can you comment on the fact that the behavior does not match the docs and was different up to the breaking change? These are things I explained in my OP.

@sanmai-NL
Copy link
Contributor Author

@fisker This a reminder. My team is waiting for this.

@fisker
Copy link
Sponsor Member

fisker commented Nov 21, 2023

If I remember correctly, it only throws when the pattern is used as arguments.

Say you have a/b is a symblink. The error only throws when you use prettier a/b or prettier a/b someother-pattern. It doesn't throws if you use prettier a or prettier "a/**/*". So, why are you using it as argument?

Correct me if I'm wrong.

@sanmai-NL
Copy link
Contributor Author

This was clarified in #14627 (comment).

A tool that uses e.g., Git index changes to trigger Prettier cannot filter for symlinks in a cross-OS compatible manner.

@fisker
Copy link
Sponsor Member

fisker commented Nov 21, 2023

Put it behind --no-error-on-unmatched-pattern then. #15533 (comment)

@sanmai-NL
Copy link
Contributor Author

sanmai-NL commented Nov 21, 2023

What do you mean with ‘it’? Do you mean changing this PR to fix --no-error-on-unmatched-pattern or just using --no-error-on-unmatched-pattern in the first place?

This invocation throws a fault - we have been using that parameter all along. This is also discussed in the PR, that you wanted me to open a new issue for.

│ [lefthook] executing: prettier --list-different --no-error-on-unmatched-pattern --ignore-unknown -- .lefthook.yml .mega-linter.yml .sourcery.yaml .v8rrc.yml
┃  prettier ❯ 

[error] Explicitly specified pattern ".lefthook.yml" is a symbolic link.
[error] Explicitly specified pattern ".mega-linter.yml" is a symbolic link.
[error] Explicitly specified pattern ".sourcery.yaml" is a symbolic link.
[error] Explicitly specified pattern ".v8rrc.yml" is a symbolic link.

@fisker
Copy link
Sponsor Member

fisker commented Nov 21, 2023

I think the behavior is expected, but maybe we can skip it if --no-error-on-unmatched-pattern is used.

NOT THROW IF --no-error-on-unmatched-pattern.

@sosukesuzuki
Copy link
Member

@fisker Please allow me to ask a question for clarification. Is what you are meaning following?

It is expected behavior that Prettier throws an error when you directly specify the symbolic link path.
However, we can accept pull requests that change that it does not throw errors when --no-error-on-unmatched-pattern is enabled.

@fisker
Copy link
Sponsor Member

fisker commented Nov 22, 2023

Yes, that's what I expect, what do you think?

@sosukesuzuki
Copy link
Member

I'm 👍 on your opinion.

@sanmai-NL
Copy link
Contributor Author

@sosukesuzuki I implemented the requested change. Can you review please?

changelog_unreleased/cli/15533.md Outdated Show resolved Hide resolved
src/cli/expand-patterns.js Outdated Show resolved Hide resolved
@fisker
Copy link
Sponsor Member

fisker commented Nov 30, 2023

@sanmai-NL @andersk Please take a look if you are fine with the changes.

@sanmai-NL
Copy link
Contributor Author

I'm fine with them too.

@sanmai-NL
Copy link
Contributor Author

Should @sosukesuzuki still review this? Hopefully users like in #15723 can be helped out soon. The PR is very small and @fisker made some fixes and improvements already.

@sanmai-NL sanmai-NL changed the title Skip symlink patterns, rather than raise a fault Allow skipping symlink patterns, to avoid raising a fault Dec 6, 2023
@junaga
Copy link

junaga commented Dec 6, 2023

  1. resolve all symlinks
  2. add an option for no symlink resolving

you are not going to patch every existing tool in the world into prettier lmao.

Copy link
Member

@sosukesuzuki sosukesuzuki left a comment

Choose a reason for hiding this comment

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

Sorry for delay my review, LGTM

@sosukesuzuki sosukesuzuki merged commit 8e816ad into prettier:main Dec 7, 2023
28 checks passed
@sanmai-NL sanmai-NL deleted the Recover-from-breaking-change-to-symlink-handling branch December 8, 2023 08:31
medikoo pushed a commit to medikoo/prettier-elastic that referenced this pull request Feb 16, 2024
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.

None yet

5 participants