Skip to content

[[FIX]] change escape-sequence handler for double quotes (\")#3566

Merged
jugglinmike merged 4 commits intojshint:masterfrom
emonmeena:escap_sequence_handling
Sep 12, 2021
Merged

[[FIX]] change escape-sequence handler for double quotes (\")#3566
jugglinmike merged 4 commits intojshint:masterfrom
emonmeena:escap_sequence_handling

Conversation

@emonmeena
Copy link
Copy Markdown
Contributor

@emonmeena emonmeena commented Aug 26, 2021

Changes the handling of escape sequence case for \" in the lex.js file. Earlier, it converted \" into \\\", which is not the correct js equivalent for the case as it should be " only. This was causing issue #3315 as \" != " that is why jshint considers them different strings.

Fixes and Closes #3315

@coveralls
Copy link
Copy Markdown

coveralls commented Aug 26, 2021 •
edited
Loading

Coverage Status

Coverage remained the same at 100.0% when pulling 1a87b62 on maayami:escap_sequence_handling into 4a681b9 on jshint:master.

jugglinmike
jugglinmike requested changes Aug 29, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@jugglinmike jugglinmike left a comment

Choose a reason for hiding this comment

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

Thanks for the patch! Could you add a unit test which demonstrates its correctness? The file tests/unit/core.js seems like a good place for this. We could use a test case that fails in the current release of JSHint but passes with this patch applied. That will prove that this patch has the intended effect, and it will help us avoid accidentally breaking the new and improved behavior in the future.

Comment thread src/lex.js
@emonmeena
Copy link
Copy Markdown
Contributor Author

emonmeena commented Aug 29, 2021 •
edited
Loading

Sure @jugglinmike I'll add a unit test for this.

@emonmeena
Copy link
Copy Markdown
Contributor Author

emonmeena commented Aug 29, 2021 •
edited
Loading

Hey @jugglinmike, I have added a unit test in the commit 849e08d that fails for the latest version of JSHint but passes with this path applied. I have checked for the existing unit tests as well, it has passed all of them.

Also, can you please check the changes in the commit 1a87b62. It deals with the naming of the unit test.

@emonmeena emonmeena requested a review from jugglinmike August 29, 2021 18:13
@jugglinmike
Copy link
Copy Markdown
Member

jugglinmike commented Sep 12, 2021

Beautiful! Thanks for the help!

@jugglinmike jugglinmike merged commit 75e48b7 into jshint:master Sep 12, 2021
@josephggd josephggd mentioned this pull request Jun 20, 2024
Open
@bardicknowledge bardicknowledge mentioned this pull request Sep 11, 2024
Open
@jalamangkara jalamangkara mentioned this pull request Sep 19, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jugglinmike jugglinmike jugglinmike approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

" and \" are considered as different keys

3 participants

@emonmeena @coveralls @jugglinmike