base reporter: Strip commas from the JSON.stringify output before diffing

[papandreou]

(This is a less controversial version of #1181 because it doesn't introduce a new dependency)

This produces more compact output and prevents lines that only differ due to a dangling comma from showing up.

Before you would get a diff like this when the ASCIIbetically last property is missing from e.actual:

    + expected - actual

     {
       "a": 123,
    +  "b": 456
    -  "b": 456,
    -  "c": 789
     }

With this patch you'll get:

    + expected - actual

     {
       "a": 123
       "b": 456
    -  "c": 789
     }

[travisjeffery]

now the json output looks off without commas

[papandreou]

Yes, maybe a little, but combined with the canonicalization of objects, this PR makes the object diffs actually usable with no "false positive" colored lines, so I still think it's a big net gain. I for one have already grown used to not having the commas in the output.

I guess putting commas back in after diffing is not an option either, and making the diff algorithm exclude trailing commas from what's being diffed would also lead to weirdness in some cases, such as:

     {
       "a": 123,
       "b": 456,
    -  "c": 789
     }

Would that still be better in your opinion?

It would of course be better to get "real" object diffs such as https://github.com/substack/difflet, but previous efforts (#18 and #589) to add that never went anywhere.

[rlidwka]

Maybe add trailing comma to all objects instead of removing all commas?

[papandreou]

I implemented a JSON diff in the jsdiff module: kpdecker/jsdiff#28

[jbnicolai]

I like it, and don't really consider the lack of a trailing comma in the diff to be a problem. @travisjeffery, perhaps a second thought? :)

[travisjeffery]

it's simple to do. like @rlidwka said #1182 (comment). that should be included in this pr

[papandreou]

I don't totally agree, but it's still much better than the current diffs, so I implemented @rlidwka's suggestion. I hope this can be merged now.

[papandreou]

@travisjeffery I just realized that you pulled in my original proposal that strips all commas rather than the later change that adds dangling commas -- and added a test for it. I'm not going to object against that as I still think it's the better solution. Just checking whether it was intentional?

[travisjeffery]

ya it was intentional