-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Conversation
…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 }
now the json output looks off without commas |
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:
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. |
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
Maybe add trailing comma to all objects instead of removing all commas? |
I implemented a JSON diff in the jsdiff module: kpdecker/jsdiff#28 |
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? :) |
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, }
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. |
@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? |
ya it was intentional |
(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
:With this patch you'll get: