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]] change escape-sequence handler for double quotes (\") #3566

Merged
merged 4 commits into from Sep 12, 2021

Conversation

emonmeena
Copy link
Contributor

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

coveralls commented Aug 26, 2021

Coverage Status

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

Copy link
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.

src/lex.js Show resolved Hide resolved
@emonmeena
Copy link
Contributor Author

emonmeena commented Aug 29, 2021

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

@emonmeena
Copy link
Contributor Author

emonmeena commented Aug 29, 2021

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.

@jugglinmike
Copy link
Member

Beautiful! Thanks for the help!

@jugglinmike jugglinmike merged commit 75e48b7 into jshint:master Sep 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

" and \" are considered as different keys
3 participants