mathiasbynens / jsesc

Given some data, jsesc returns the shortest possible stringified & ASCII-safe representation of that data.
https://mths.be/jsesc
MIT License
716 stars 53 forks source link

Add an option to allow lowercase hexadecimal. #23

Closed almet closed 8 years ago

almet commented 8 years ago

Here is a way to lowercase the generated codepoints (useful for python compatibility for instance).

mathiasbynens commented 8 years ago

Why should this be added?

Python compatibility is not an issue since Python supports both \xE9 and \xe9, to use your example.

almet commented 8 years ago

Thanks for your answer. Actually, python supports lowercase and uppercase hexadecimal as input, but not as output: it always outputs lowercase.

My use case is to be able to serialize JSON in a canonical way between python and JavaScript (at first), and for this I need to have the same representation of unicode chars (otherwise the canonical form differs).

One way is of course to change the output of python. I think both approach are valid, and thought that this use case could fit others needs, hence the contribution.

Is the intent clearer here?

mathiasbynens commented 8 years ago

I left some inline comments. The documentation would need to be updated as well.

almet commented 8 years ago

I tried to address your concerns, let me know if that's enough, I'm happy to do some more changes if needed.

mathiasbynens commented 8 years ago

Looking good so far!

Nit: let’s go for uppercaseHex instead of upperCaseHex since “uppercase” is one word.

Thought of some more things — sorry I forgot to mention these earlier:

almet commented 8 years ago

Now that I think about it, maybe the option should be lowercaseHex, because by default it's upper case. What do you think?

almet commented 8 years ago

I've just updated the option to expose lowercaseHex rather than uppercaseHex.

mathiasbynens commented 8 years ago

Good call on lowercaseHex vs uppercaseHex!

almet commented 8 years ago

Updated!

almet commented 8 years ago

Hey! Thanks for your time again. Anything else I can do to land this?

mathiasbynens commented 8 years ago

LGTM with those minor nits addressed + commits squashed. :+1:

almet commented 8 years ago

Just updated and squashed!

mathiasbynens commented 8 years ago

Are you up for adding those tests? If not, I’ll merge this and do it myself later before releasing a new version.

almet commented 8 years ago

Oh, this is because of the change of setting! Is it okay to switch them to true for all the tests added (that was the original intent)

almet commented 8 years ago

I've updated the tests with the two cases "true" and "false". Let me know if this is enough :)

almet commented 8 years ago

Updated.

mathiasbynens commented 8 years ago

I’ll merge and fix those nits myself.

mathiasbynens commented 8 years ago

Thanks!

mathiasbynens commented 8 years ago

v1.0.0 includes this change.