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

Implemented diffJson #28

Merged
merged 7 commits into from
Nov 29, 2014
Merged

Implemented diffJson #28

merged 7 commits into from
Nov 29, 2014

Conversation

papandreou
Copy link
Contributor

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

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
@papandreou papandreou changed the title Implemented diffJson. Implemented diffJson Apr 3, 2014
@rooftopsparrow
Copy link

Bump...
I would love to see this in the mainline.

However, it would be great if it accepted an already stringified JSON as well.

@papandreou
Copy link
Contributor Author

@rooftopsparrow There, that was easy enough :)

In that case it's the caller's duty to stringify the arguments with the same indentation level -- and to canonicalize the objects before stringifying them.

@rooftopsparrow
Copy link

Fabulous! Works like a charm.


var canonicalizedObj;

if ('[object Array]' == {}.toString.call(obj)) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The toString method should be cached. Using the object literal adds a bit of overhead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point -- fixed.

@kpdecker
Copy link
Owner

kpdecker commented Sep 2, 2014

This generally looks pretty good. A few minor questions and generally wondering if those questions should have comments in the code explaining them but outside of that I'd like to get this into master.

(BTW the travis environment failure should hopefully be fixed in master)

@papandreou
Copy link
Contributor Author

If my explanations are good enough, we can add them as comments in the code :)

@kpdecker kpdecker merged commit d12c46d into kpdecker:master Nov 29, 2014
@kpdecker
Copy link
Owner

I've fixed the merge conflicts and got this in. Thanks for writing this!

@kpdecker
Copy link
Owner

Released in 1.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.

3 participants