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

Resolve issue73 where quotes in character would trigger string state #75

Merged
merged 2 commits into from May 9, 2019

Conversation

boyter
Copy link
Owner

@boyter boyter commented May 5, 2019

A fix for #73

If a " was used inside a character it would trigger the string state. Quick fix is to check for \ before it and stay in the code state.

Long term fix is to have a state for dealing with quotes, perhaps inside strings as they have the same rules. However this requires updating the whole JSON document and checking that quotes work like strings in all languages (unlikely). This seems like a good reasonable short term fix for most C like languages.

@dbaggerman
Copy link
Collaborator

I'm a little concerned that not all languages follow exactly the same quoting rules, and hardcoding the backslash like this might break other languages.

PowerShell for example uses a backtick rather than a backslash to escape characters, as far as I'm aware.

There are also cases like backtick strings in Go, where escape characters can't be used at all.

fmt.Println(`\`)

will print \

@boyter
Copy link
Owner Author

boyter commented May 7, 2019

Absolutely. Its a short term thing that should catch most cases for C like languages but can then be improved with #76 This is to get it in place then improve. I also want to change the return from the states to a struct now because its getting hard to read, but thats also going on that other branch.

In your Go example it would hit the string state first and be counted correctly so no issues there thankfully.

Certainly not intending to get this into a release, however it lays the ground work for the other branch.

@dbaggerman
Copy link
Collaborator

Agreed that it probably isn't ready for a release as is, but as a starting point it's fine.

@boyter boyter merged commit 3aab30a into master May 9, 2019
@boyter boyter deleted the issue73 branch May 9, 2019 01:39
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.

None yet

2 participants