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

no-irregular-whitespace: Inside of regexp irregular whitespace #5840

Closed
terrierscript opened this issue Apr 13, 2016 · 12 comments
Closed

no-irregular-whitespace: Inside of regexp irregular whitespace #5840

terrierscript opened this issue Apr 13, 2016 · 12 comments
Assignees
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 enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules

Comments

@terrierscript
Copy link

terrierscript commented Apr 13, 2016

What version of ESLint are you using?
2.7.0

What parser (default, Babel-ESLint, etc.) are you using?
default

Please show your full configuration:

{
    "env": {
        "node": true,
    },
    "rules": {
        "no-irregular-whitespace": 2
    }
};

What did you do? Please include the actual source code causing the issue.

I wrote those code.

var reg = /^[\s\ ]*$/
reg.test('foo')

What did you expect to happen?

  1:5  error  Parsing error: Unexpected token reg

I think this may bug because string literal is ignored on this rule.

If this is not bug, I want documentation or skipRegexp option.

Note: I can avoid this error with new RegExp('^[\s\ ]*$')

@eslintbot eslintbot added the triage An ESLint team member will look at this issue soon label Apr 13, 2016
@mysticatea mysticatea added bug ESLint is working incorrectly rule Relates to ESLint's core rules evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Apr 14, 2016
@mysticatea
Copy link
Member

Thank you for this issue.

Indeed, no-irregular-whitespace ignores spaces in string literals.
So I feel this is a bug.

image

@kaicataldo
Copy link
Member

kaicataldo commented Apr 15, 2016

I actually don't think this is a bug - seems to be working as intended, as the rule states it causes warnings if irregular whitespace characters are found outside of strings or comments. From the docs:

With this rule enabled the following characters will cause warnings outside of strings and comments ...

I think that it's not working for your first example because it's a regex literal (not a string). The reason new RegExp('^[\s\ ]*$') doesn't warn is because the irregular whitespace is in a string. Or that's my interpretation of the rules docs, anyway :)

@terrierscript
Copy link
Author

Oh sorry, my explanation was lacking.

If this is regular behavior, I think this should documentation with incorrect example.
But I wish fix this behavior or append ignore option.

Because regexp with Ideographic Space' pattern is often used in multi byte language.

@kaicataldo
Copy link
Member

Got it - I'm going to label this as an enhancement and let's see what @nzakas thinks. Thanks for the issue report!

@kaicataldo kaicataldo added enhancement This change enhances an existing feature of ESLint and removed bug ESLint is working incorrectly labels Apr 15, 2016
@mysticatea
Copy link
Member

Hmm, I cannot find reasons that irregular spaces in strings are good but irregular spaces in regular expressions are bad. And I got aware that template literals have the same issue:

/^[\s\ ]*$/             /*error Irregular whitespace not allowed (no-irregular-whitespace)*/
new RegExp(`^[\s\ ]*$`) /*error Irregular whitespace not allowed (no-irregular-whitespace)*/
new RegExp("^[\s\ ]*$") // good.

I wish to fix this behavior since this does not have consistency, IMO.

What about adding options for each type?

{
    "no-irregular-whitespace": ["error", {
        "skipStrings": false,
        "skipTemplates": false,
        "skipRegExps": false,
        "skipComments": false // "skipComments" exists already.
    }]
}

In my ideal, all default are false, but it's a breaking change.

@nzakas
Copy link
Member

nzakas commented Apr 18, 2016

Irregular whitespace in strings might be intentional for output. No objections to adding the options if @mysticatea wants to as long as the default behavior remains the same.

@nzakas nzakas added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Apr 18, 2016
@mysticatea mysticatea self-assigned this Apr 18, 2016
@rhysd
Copy link
Contributor

rhysd commented Apr 30, 2016

I just encountered this bug today and sent a PR for this in #6018. Could you check it?

rhysd added a commit to rhysd/eslint that referenced this issue Apr 30, 2016
Problem:
  no-irregal-whitespace ignores string literals correctly, but doen't
  regular expressions.

Solution:
  also check that the node is a regular expression in
  removeInvalidNodeErrorsInIdentifierOrLiteral().
@mysticatea
Copy link
Member

@rhysd Thank you for the PR 😄. But the consensus on this issue is to add options above. Could you update the PR with the options?

@rhysd
Copy link
Contributor

rhysd commented Apr 30, 2016

@mysticatea I'll fix my PR today 👍

@rhysd
Copy link
Contributor

rhysd commented Apr 30, 2016

        "skipStrings": false,
        "skipTemplates": false,

Should we separate these options? I think skipTemplates should be unified into skipStrings. What do you think?

@pedrottimark
Copy link
Member

@rhysd If we were designing the rule today, unlikely to find a use case to separate options for string literals from template literals. The reason is to avoid breaking existing behavior, as requested in #5840 (comment).

Therefore, I think the default values of the options are as follows:

{
    "no-irregular-whitespace": ["error", {
        "skipStrings": true, // current default is to skip strings
        "skipTemplates": false,
        "skipRegExps": false,
        "skipComments": false // this option already exists
    }]
}

@rhysd
Copy link
Contributor

rhysd commented Apr 30, 2016

@pedrottimark

OK, I'll maintain the previous behavior 👍

rhysd added a commit to rhysd/eslint that referenced this issue Apr 30, 2016
Problem:
  no-irregal-whitespace ignores string literals correctly, but doen't
  regular expressions. (eslint#5840)
  Added below options to no-irregal-whitespace

  - skipStrings (default: true)
  - skipRegExps (default: false)
  - skipTemplates (default: true)

Solution:
  also check that the node is a regular expression in
  removeInvalidNodeErrorsInIdentifierOrLiteral().
rhysd added a commit to rhysd/eslint that referenced this issue Apr 30, 2016
Problem:
  no-irregal-whitespace ignores string literals correctly, but doen't
  regular expressions. (eslint#5840)

Solution:
  Also check that the node is a regular expression in
  removeInvalidNodeErrorsInIdentifierOrLiteral().

  Added below options to no-irregal-whitespace

  - skipStrings (default: true)
  - skipRegExps (default: false)
  - skipTemplates (default: false)
rhysd added a commit to rhysd/eslint that referenced this issue Apr 30, 2016
)

Problem:
  no-irregal-whitespace ignores string literals correctly, but doen't
  regular expressions. (eslint#5840)

Solution:
  Also check that the node is a regular expression in
  removeInvalidNodeErrorsInIdentifierOrLiteral().

  Added below options to no-irregal-whitespace

  - skipStrings (default: true)
  - skipRegExps (default: false)
  - skipTemplates (default: false)
rhysd added a commit to rhysd/eslint that referenced this issue Apr 30, 2016
)

Problem:
  no-irregal-whitespace ignores string literals correctly, but doen't
  regular expressions. (eslint#5840)

Solution:
  Also check that the node is a regular expression in
  removeInvalidNodeErrorsInIdentifierOrLiteral().

  Added below options to no-irregal-whitespace

  - skipStrings (default: true)
  - skipRegExps (default: false)
  - skipTemplates (default: false)
rhysd added a commit to rhysd/eslint that referenced this issue May 2, 2016
)

Problem:
  no-irregal-whitespace ignores string literals correctly, but doen't
  regular expressions. (eslint#5840)

Solution:
  Also check that the node is a regular expression in
  removeInvalidNodeErrorsInIdentifierOrLiteral().

  Added below options to no-irregal-whitespace

  - skipStrings (default: true)
  - skipRegExps (default: false)
  - skipTemplates (default: false)
rhysd added a commit to rhysd/eslint that referenced this issue May 2, 2016
)

Problem:
  no-irregal-whitespace ignores string literals correctly, but doen't
  regular expressions. (eslint#5840)

Solution:
  Also check that the node is a regular expression in
  removeInvalidNodeErrorsInIdentifierOrLiteral().

  Added below options to no-irregal-whitespace

  - skipStrings (default: true)
  - skipRegExps (default: false)
  - skipTemplates (default: false)
rhysd added a commit to rhysd/eslint that referenced this issue May 2, 2016
)

Problem:
  no-irregal-whitespace ignores string literals correctly, but doen't
  regular expressions. (eslint#5840)

Solution:
  Also check that the node is a regular expression in
  removeInvalidNodeErrorsInIdentifierOrLiteral().

  Added below options to no-irregal-whitespace

  - skipStrings (default: true)
  - skipRegExps (default: false)
  - skipTemplates (default: false)
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018
@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 Feb 6, 2018
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 enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules
Projects
None yet
Development

No branches or pull requests

7 participants