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

CLI: Fix failure on dir with trailing slash #11000

Merged
merged 6 commits into from Jun 11, 2021
Merged

Conversation

fisker
Copy link
Member

@fisker fisker commented Jun 1, 2021

Description

Fixes #9300
Fixes #10964

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 marked this pull request as ready for review Jun 1, 2021
@@ -76,10 +76,11 @@ async function* expandPatternsInternal(context) {
input: pattern,
});
} else if (stat.isDirectory()) {
const relativePath = path.relative(cwd, absolutePath) || ".";
Copy link
Member

@thorn0 thorn0 Jun 4, 2021

Choose a reason for hiding this comment

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

If the goal of this line is only to remove trailing slashes, why not do .replace(/[/\\]+$/, '') here to express the intent more clearly?

Copy link
Member Author

@fisker fisker Jun 4, 2021

Choose a reason for hiding this comment

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

I think the middle one foo//bar was effected too.

Copy link
Member Author

@fisker fisker Jun 4, 2021

Choose a reason for hiding this comment

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

I'll confirm when I back to laptop, if it's true, I'll add comment.

Copy link
Member Author

@fisker fisker Jun 4, 2021

Choose a reason for hiding this comment

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

Tested, the tailing slash is the only problem, applied your suggestion.

Copy link
Member Author

@fisker fisker Jun 4, 2021

Choose a reason for hiding this comment

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

Your solution can't work, because \ can be the dirname, see this log https://github.com/prettier/prettier/runs/2746762536#step:6:1251

This commit da46196 works, but feel better/safer to use path.relative, added comments b009878

thorn0
thorn0 approved these changes Jun 4, 2021
@fisker fisker merged commit ce96e48 into prettier:main Jun 11, 2021
25 checks passed
@fisker fisker deleted the pattern-slash branch Jun 11, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants