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

refactor: use / separator when adjusting ignorePatterns on Windows #18613

Merged
merged 2 commits into from
Jun 21, 2024

Conversation

mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Jun 20, 2024

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[x] Other, please explain:

When converting ignorePatterns (--ignore-pattern), which are relative to cwd, to patterns relative to basePath, we were prepending path.relative(basePath, cwd).

On Windows, this was producing a mix of \ and / as path separators in the result. For example, if the config file is in the root, and you run eslint --ignore-pattern a.js in a subfolder src\app, the ignore pattern passed to the config array would be src\app/a.js.

What changes did you make? (Give an overview)

Added .replaceAll(path.sep, "/") to the relative path to replace \ with / on Windows and thus consistently produce ignore patterns with / separators.

Is there anything you'd like reviewers to focus on?

I marked this as a refactor because the previous code currently doesn't cause any bugs, but it would cause bugs after we update config-array to always treat \ as an escape character (eslint/rewrite#61).

@mdjermanovic mdjermanovic requested a review from a team as a code owner June 20, 2024 13:23
@eslint-github-bot eslint-github-bot bot added the chore This change is not user-facing label Jun 20, 2024
Copy link

netlify bot commented Jun 20, 2024

Deploy Preview for docs-eslint canceled.

Name Link
🔨 Latest commit 69a6fdb
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/6674963ae7e23a0008de9326

@github-actions github-actions bot added cli Relates to ESLint's command-line interface core Relates to ESLint's core APIs and features labels Jun 20, 2024
@@ -421,7 +421,7 @@ async function calculateConfigArray(eslint, {
relativeIgnorePatterns = ignorePatterns;
} else {

const relativeIgnorePath = path.relative(basePath, cwd);
const relativeIgnorePath = path.relative(basePath, cwd).replaceAll(path.sep, "/");
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a comment here that summarizes why we are doing this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added in 69a6fdb

@amareshsm amareshsm added the accepted There is consensus among the team that this change meets the criteria for inclusion label Jun 20, 2024
Copy link
Contributor

@snitin315 snitin315 left a comment

Choose a reason for hiding this comment

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

LGTM. I'll leave it open for others to review.

Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

LGTM.

@nzakas nzakas merged commit d2d06f7 into main Jun 21, 2024
19 checks passed
@nzakas nzakas deleted the ignorepatterns-backslash branch June 21, 2024 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion chore This change is not user-facing cli Relates to ESLint's command-line interface core Relates to ESLint's core APIs and features
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants