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

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

Closed
wants to merge 2 commits into from

Conversation

papandreou
Copy link
Contributor

(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
     }

…fing.

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
Copy link
Contributor

now the json output looks off without commas

@papandreou
Copy link
Contributor Author

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.

papandreou added a commit to papandreou/jsdiff that referenced this pull request Apr 3, 2014
It takes two objects, serializes them as canonical JSON, then does a line-based diff that ignores differences in trailing commas.

See discussion here: mochajs/mocha#1182
@rlidwka
Copy link

rlidwka commented Apr 3, 2014

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

@papandreou
Copy link
Contributor Author

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

@jbnicolai
Copy link

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
Copy link
Contributor

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

…s suggestion.

Now you'll get output like this instead:

    + expected - actual

     {
       "a": 123,
       "b": 456,
    -  "c": 789,
     }
@papandreou
Copy link
Contributor Author

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
Copy link
Contributor Author

@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
Copy link
Contributor

ya it was intentional

tandrewnichols pushed a commit to tandrewnichols/mocha that referenced this pull request Dec 15, 2014
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.

4 participants