Skip to content

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

Merged
nzakas merged 2 commits intomainfrom
ignorepatterns-backslash
Jun 21, 2024
Merged

refactor: use / separator when adjusting ignorePatterns on Windows#18613
nzakas merged 2 commits intomainfrom
ignorepatterns-backslash

Conversation

@mdjermanovic
Copy link
Copy Markdown
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
@netlify
Copy link
Copy Markdown

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
Comment thread lib/eslint/eslint.js
} else {

const relativeIgnorePath = path.relative(basePath, cwd);
const relativeIgnorePath = path.relative(basePath, cwd).replaceAll(path.sep, "/");
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
@nzakas nzakas deleted the ignorepatterns-backslash branch June 21, 2024 13:45
@eslint-github-bot eslint-github-bot Bot locked and limited conversation to collaborators Dec 19, 2024
@eslint-github-bot eslint-github-bot Bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Dec 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion 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