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.

Sign up for GitHub

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

Jump to bottom

no-control-regex error should indicate which unexpected chars were found #6293

Closed
ljharb opened this issue May 31, 2016 · 13 comments
Closed

no-control-regex error should indicate which unexpected chars were found #6293

ljharb opened this issue May 31, 2016 · 13 comments
Assignees
@platinumazure
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

@ljharb
Copy link
Sponsor Contributor

ljharb commented May 31, 2016 •
edited

What version are you using?
2.11.1

What did you do?
enabled no-control-regex and ran it on https://github.com/es-shims/String.prototype.trim/blob/e89adeefcbd6fb59563e62d230b8036d65a8bf69/implementation.js#L7-L8

What happened?
Got "Unexpected control character in regular expression"

What did you expect to happen?
I'd like to know which control character was unexpected, so I can quickly fix it.
@eslintbot eslintbot added the triage An ESLint team member will look at this issue soon label May 31, 2016
@vitorbal vitorbal added enhancement This change enhances an existing feature of ESLint 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 May 31, 2016
@vitorbal
Copy link
Member

vitorbal commented May 31, 2016 •
edited

Hi @ljharb, thanks for the issue! What you are proposing is an improvement to the error message, correct? I totally agree this would be helpful.
What do you think about showing the column position of the unexpected character as well?

@ljharb
Copy link
Sponsor Contributor Author

ljharb commented May 31, 2016

Yes, an improvement to the error message. Column position would be great too but imo the more important thing is the character itself.

@pedrottimark
Copy link
Member

@ljharb Since the problem is a control character, are you thinking of the character (which might be invisible) or its code, or both? If the code, in hexadecimal or decimal, or both?

@ljharb
Copy link
Sponsor Contributor Author

ljharb commented May 31, 2016

@pedrottimark the escaped form, ie, \x03 or whatever - in other words, the literal thing i typed into the code that caused the warning.

This was referenced Jun 16, 2016
Closed
Closed
@nzakas nzakas added the rule Relates to ESLint's core rules label Aug 3, 2016
@nzakas
Copy link
Member

nzakas commented Aug 3, 2016

@eslint/eslint-team is anyone willing to champion this change?

@ljharb
Copy link
Sponsor Contributor Author

ljharb commented Aug 3, 2016

I'm still attempting to implement it, if that helps it stay open. I was blocked on #6752 being merged, so it's only been 6 days since then :-)

ljharb added a commit to ljharb/eslint that referenced this issue Aug 4, 2016
@ljharb
Update: improve error message in no-control-regex.
f78532f 
Fixes eslint#6293.
@ljharb
Copy link
Sponsor Contributor Author

ljharb commented Aug 4, 2016

I have the pull request ready, but since this issue isn't yet marked as accepted, it seems like I shouldn't file it yet.

@mikesherov
Copy link
Contributor

👍 on this change.

@platinumazure
Copy link
Member

I'll champion.

@platinumazure platinumazure self-assigned this Aug 4, 2016
@ilyavolodin
Copy link
Member

Sounds good to me. 👍

@ljharb ljharb mentioned this issue Aug 4, 2016
Merged
ljharb added a commit to ljharb/eslint that referenced this issue Aug 4, 2016
@ljharb
Update: improve error message in no-control-regex (fixes eslint#6293)
90b19dd
ljharb added a commit to ljharb/eslint that referenced this issue Aug 4, 2016
@ljharb
Update: improve error message in no-control-regex (fixes eslint#6293)
25df5cc
@platinumazure
Copy link
Member

Open question: Do we have consensus that we'll just tweak the error message for now, no location tweaks? (Location info would require reporting one violation per character instead of per literal.)

I'm 👎 (but not strongly) to location info, because I think the characters showing up in the lint message is already a huge win for 99% of use cases. If users want line/column info and create an issue to that effect, we can reconsider then. That's just my thought on the matter, others may disagree.

ljharb added a commit to ljharb/eslint that referenced this issue Aug 4, 2016
@ljharb
Update: improve error message in no-control-regex (fixes eslint#6293)
e8b7bd2
ljharb added a commit to ljharb/eslint that referenced this issue Aug 5, 2016
@ljharb
Update: improve error message in no-control-regex (fixes eslint#6293)
1f4a48c
@ilyavolodin
Copy link
Member

@eslint/eslint-team Can we get one more 👍 here to mark this as accepted? We have a PR that already has been code reviewed and just waiting for this issue to be accepted.

@btmills
Copy link
Member

btmills commented Aug 9, 2016

Makes sense to me 👍

@platinumazure platinumazure 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 Aug 9, 2016
ljharb added a commit to ljharb/eslint that referenced this issue Aug 9, 2016
@ljharb
Update: improve error message in no-control-regex (fixes eslint#6293)
ec1c996
ljharb added a commit to ljharb/eslint that referenced this issue Aug 9, 2016
@ljharb
Update: improve error message in no-control-regex (fixes eslint#6293)
bcd3f1c
ljharb added a commit to ljharb/eslint that referenced this issue Aug 9, 2016
@ljharb
Update: improve error message in no-control-regex (fixes eslint#6293)
f1844a3
ljharb added a commit to ljharb/eslint that referenced this issue Aug 9, 2016
@ljharb
Update: improve error message in no-control-regex (fixes eslint#6293)
1c4910a
ilyavolodin pushed a commit that referenced this issue Aug 10, 2016
@ljharb @ilyavolodin
Update: improve error message in no-control-regex (#6839)
1ecd2a3 
* Update: improve error message in `no-control-regex` (fixes #6293)

* fixup: this will be squashed before the PR is merged

* fixup: this will be squashed before the PR is merged

* fixup: Add more test cases; fix a bug.

* fixup: try to improve variable names.
@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.
Assignees

@platinumazure platinumazure

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
Milestone
No milestone
Development

No branches or pull requests
9 participants
@nzakas @ljharb @platinumazure @mikesherov @vitorbal @ilyavolodin @btmills @pedrottimark @eslintbot