Use string processing instead of regex #14
Conversation
|
@ruipneves Could do update this PR to obey the code style? See: https://travis-ci.org/sindresorhus/to-single-quotes/jobs/193572769 If you are using atom or sublime search for Same thing applies to sindresorhus/round-to#6 PR. |
|
I pushed the fix for the linter-xo but it has a problem for node version 0.10 and 0.12 as you can see in Travis log. This is due to the xo library being installed with the latest version which causes an error by using const which is not supported in versions 0.10 and 0.12. |
|
@ruipneves You are right, we probably need to remove support for |
| var x, y, len = str.length; | ||
| var result = ''; | ||
| var x; | ||
| var len = str.length; | ||
| var betweenQuotes = false; | ||
| var QuoteChar; |
| var betweenQuotes = false; | ||
| var QuoteChar; | ||
| for (x = 0; x < len; x++) { | ||
| // FOUND DOUBLE-QUOTE OR SINGLE-QUOTE |
| // FOUND DOUBLE-QUOTE OR SINGLE-QUOTE | ||
| if (str[x] === '"' || str[x] === '\'') { | ||
| // IF NOT PROCESSING IN BETWEEN QUOTES | ||
| if (betweenQuotes === false) { |
There was a problem hiding this comment.
I had !betweenQuotes on the initial version but linter-xo pointed an error there when I ran the fix. Maybe the chain of issues below caused linter-xo to report that as an error as well?
| // ESCAPE + DOUBLE QUOTE | ||
| result += '"'; | ||
| x++; | ||
| } else { |
There was a problem hiding this comment.
There are some strange indentation issues going on.
See: https://github.com/ruipneves/to-single-quotes/blob/20960f7b6cfdcd4e04d1e6819d8f4437cc23aa4a/index.js#L22 https://github.com/ruipneves/to-single-quotes/blob/20960f7b6cfdcd4e04d1e6819d8f4437cc23aa4a/index.js#L35 etc
|
@ruipneves I've added a few comments, sorry for the delay. |
|
@satazor Thank you for your feedback. I just pushed the fix for the code styling errors. I still don't know how the strange indentations got there. The only thing I did in the previous submission was applying the linter-xo fix to the code and manually fixing the errors that linter-xo couldn't fix automatically. |
There was a problem hiding this comment.
Leaving this out for @sindresorhus to approve the code logic
|
@sindresorhus tests are failing on node < 4 because of xo, is it ok to remove support for older nodes right? |
|
That or set XO to a fixed version. If I'm not mistaken, it should be |
👍 I will remove support for Node.js 0.10/0.12 after this PR is merged. |
sindresorhus
changed the title
| .replace(/^"|"$/g, '\''); | ||
| }); | ||
| var result = ''; | ||
| var x; |
| }); | ||
| var result = ''; | ||
| var x; | ||
| var len = str.length; |
There was a problem hiding this comment.
There's no point in caching the length. V8 is smart enough to do it for us.
| result += str[x]; | ||
| } | ||
| } else if (str[x] === '\\' && str[x + 1] === '\\') { | ||
| // backslashes --> don't touch them |
There was a problem hiding this comment.
Make it:
don't touch backslashes
|
Looks pretty good :)
Agreed. Not as a documented option, but to be able to reuse the logic here in |
|
@sindresorhus Thank you for the feedback! Just pushed the new version with the requested changes. |
|
👍 Looks really good. Thank you @ruipneves :) |
I saw this issue and decided to fork and give it a try, mainly because regular expressions can become really messy and confusing especially if things scale. The string processing suggested by you, was definitely a good idea to straighten things up.
The solution that I found for this problem was to iterate through the string maintaining a state, which is defined by the betweenQuotes variable. It is used to know when the processing is happening between two matching quotes and when the closing quote is found. While processing between matching quotes, escape and backslashes characters are processed in order to know when to escape a single quote or simply to escape the backslash.
What do you guys think?
Any suggestions?! Feel free to give me your advice.
TODO:
Fixes #13