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 escaped whitespace in paths for config initialization (fixes #11099) #11101

Closed
wants to merge 1 commit into from

Conversation

getify
Copy link

@getify getify commented Nov 19, 2018

What is the purpose of this pull request?

[x] Bug fix

Reproduced and fixed issue #11099 with:

ESLint Version: v5.9.0
Node Version: v10.4.1

What changes did you make?

I changed the regex used in splitting the paths, to skip splitting on an escaped space. Then I remove any leftover \ characters from the split-out patterns using a .map(..).

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

Because I'm using a regex negative lookbehind, it means this config tool could only run in Node with that recent JS feature present (ES2018), which (I think) was Node 10.0+. Not sure how that impacts eslint's Node support.

Also, I was going to modify the tests for config-initializer here, but decided not to, because it's not clear if there's any reasonable way to get an internal path in eslint with a space in it to be able to test against. :/

@jsf-clabot
Copy link

jsf-clabot commented Nov 19, 2018

CLA assistant check
All committers have signed the CLA.

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Nov 19, 2018
@not-an-aardvark
Copy link
Member

Thanks for the PR!

Re. regex negative lookbehind, ESLint still needs to support Node 6 (see here for the precise supported versions), so I don't think we can use negative lookbehinds yet. Could this be refactored to avoid negative lookbehinds?

For the config-initializer tests, would it work to create a folder with a space within tests/fixtures (e.g. tests/fixtures/folder name with a space/myFile.js) and use files in that folder for the test?

@nzakas nzakas added bug ESLint is working incorrectly cli Relates to ESLint's command-line interface and removed triage An ESLint team member will look at this issue soon labels Nov 20, 2018
Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

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

Could you please add some tests? Thanks!

Also, is negative lookbehind supported in all versions of Node that ESLint supports?

@getify getify closed this Feb 22, 2019
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Aug 22, 2019
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Aug 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly cli Relates to ESLint's command-line interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants