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

Added possibility to remove solidus symbol escaping #30

Merged
merged 3 commits into from Mar 31, 2015
Merged

Added possibility to remove solidus symbol escaping #30

merged 3 commits into from Mar 31, 2015

Conversation

zaa
Copy link
Contributor

@zaa zaa commented Nov 19, 2014

According to http://json.org/, solidus symbol (forward slash) should be escaped as "/". Some JSON generators abide the rule and generate strings like "http://example.com/test".
I've added an option (-s) that provides possibility to normalize such strings and replace "/" with plain "/".

@zaa zaa mentioned this pull request Nov 19, 2014
@aidanhs
Copy link
Collaborator

aidanhs commented Nov 19, 2014

Hi, it looks like you've introduced a bashism here, which means potential incompatibility with other shells.
In particular, see https://wiki.ubuntu.com/DashAsBinSh#A.24.7Bparm.2BAC8.3F.2BAC8-pat.5B.2BAC8-str.5D.7D

(a test or two might also be nice)

@zaa
Copy link
Contributor Author

zaa commented Nov 20, 2014

As far as I can see, JSON.sh specifically asks for bash (#!/usr/bin/env bash), so that should not be a problem. Please let me know if this is not a case, I will try to come up with a workaround (though I would prefer not to add a dependency on awk or something like that).

I've added the test cases.

Thank you.

@dominictarr
Copy link
Owner

Ah I didn't know about this. I initially generated the test cases with node's JSON.stringify,
and that does not escape /... Though that is in the spec. I guess douglas thought that unescaped / is was not one of javascript's good parts.

@aidanhs would this work in sh? I tried testing this but arch only wants to give me bash when i try install sh.

@aidanhs
Copy link
Collaborator

aidanhs commented Nov 20, 2014

We already rely on gawk if egrep misbehaves -

gawk '{
(admittedly this isn't ideal, because it won't work on busybox (for example)...I might work on this).
You could just use sed, or a while loop with expr if you wanted to stick to coreutils.
Hadn't seen the shebang...I note a couple of bashisms have come back since my last effort so I guess this could be merged and I'll address it all separately.


Regarding shell compatibility, dash:

# X=12345; echo ${X/3/a}
dash: 1: Bad substitution

fish:

root@b20a0f918a36 ~# X=12345; echo ${X/3/a}
fish: Did you mean {$VARIABLE}? The '$' character begins a variable name. A bracket, which directly followed a '$', is not allowed as a part of a variable name, and variable names may not be zero characters long. To learn more about variable expansion in fish, type 'help expand-variable'.
X=12345; echo ${X/3/a}

@aidanhs
Copy link
Collaborator

aidanhs commented Nov 20, 2014

(ignore my deleted comment, I got confused)

@aidanhs
Copy link
Collaborator

aidanhs commented Nov 20, 2014

Ok, maybe I should put the comment back.
Is the last example below doing the correct thing? I'm trying to wrap my head around it:

JSON.sh $ echo '"\\"' | ./JSON.sh
[]      "\\"
JSON.sh $ echo '"\\/"' | ./JSON.sh
[]      "\\/"
JSON.sh $ echo '"\\"' | ./JSON.sh -s
[]      "\\"
JSON.sh $ echo '"\\/"' | ./JSON.sh -s
[]      "\/"

I think what's going on in that final example is that solidus escaping is happening, so one of the backslashes is removed. But it shouldn't be, because the backslash is escaped?

@zaa
Copy link
Contributor Author

zaa commented Nov 20, 2014

The thing is, a JSON generator that escapes solidus would not produce a string like: "\\/"

Example:

<?php
$a = '\\/';
echo "Original string: " . $a . "\n";
echo "JSON: " . json_encode($a) . "\n";
?>

Output:

> php solidus.php
Original string: \/
JSON: "\\\/"

JSON.sh -s correctly replaces "\\\/" with "\\/":

> cat ~/3.json
"\\\/"
> cat ~/3.json | ./JSON.sh -s
[]  "\\/"

@aidanhs
Copy link
Collaborator

aidanhs commented Nov 20, 2014

Fair point. No more objections. I'll look at bashisms (and gawk) separately.

@zaa
Copy link
Contributor Author

zaa commented Mar 29, 2015

Just a friendly reminder.
If this is not too much trouble, I would love to see this merged into the mainline.
Thank you.

@dominictarr dominictarr merged commit dcbd7ee into dominictarr:master Mar 31, 2015
@dominictarr
Copy link
Owner

merged into 0.2.0

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

3 participants