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

fix!: Allow escaping characters on Windows #61

Merged
merged 2 commits into from
Jun 24, 2024
Merged

Conversation

mdjermanovic
Copy link
Member

Prerequisites checklist

What is the purpose of this pull request?

Allows using \ as an escape character in patterns regardless of the operating system. In particular, on Windows.

What changes did you make? (Give an overview)

Enabled allowWindowsEscape minimatch option.

Related Issues

refs eslint/eslint#18597

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

Should this maybe be marked as a breaking change for this package?

@eslint-github-bot eslint-github-bot bot added the bug Something isn't working label Jun 19, 2024
@mdjermanovic
Copy link
Member Author

It seems that one test in eslint/eslint fails after this change. I'll look into this more.

@mdjermanovic mdjermanovic marked this pull request as draft June 19, 2024 16:40
@mdjermanovic
Copy link
Member Author

The problem is with --ignore-pattern: we're prepending relative path in the os-specific format (for example, --ignore-pattern a.js run in src\app on Windows becomes src\app/a.js ignore pattern in config array). I'll submit a PR in eslint/eslint to update this first.

@mdjermanovic mdjermanovic changed the title fix: Allow escaping characters on Windows fix!: Allow escaping characters on Windows Jun 20, 2024
@mdjermanovic mdjermanovic marked this pull request as ready for review June 21, 2024 14:59
@mdjermanovic
Copy link
Member Author

Now that eslint/eslint#18613 has been merged, this is ready for review. I just tagged this as fix! to bump config-schema to be out of range for eslint <= 9.5.0.

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.

And yes, I do believe this should be marked as breaking for this package.

Leaving open for others to review over the weekend.

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

@snitin315 snitin315 merged commit 8501890 into main Jun 24, 2024
14 checks passed
@snitin315 snitin315 deleted the eslint-issue18597 branch June 24, 2024 02:29
@github-actions github-actions bot mentioned this pull request Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted breaking bug Something isn't working
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants