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

utils.js: significant modification of diff behavior #1357

Merged
merged 1 commit into from
Dec 11, 2014

Conversation

boneskull
Copy link
Member

I think it's better, anyway. I was becoming frustrated by seeing my Dates and Functions represented as objects within the diffs. Even more frustrating is when the diff result appears to be exactly the same but the assertion failed anyway. So this is my attempt at making the diffs more friendly and understandable.

Summary of changes:

  • Added inner function emptyRepresentation(), which is called in the case that your Object, Function or Array has nothing "in" it. It attempts to make a unique representation of that empty value.
  • Added private API function exports.type(), which can be used to deduce the type of any given value. I exported this function mainly for testing purposes
  • Modified exports.stringify() to avoid JSON.stringify() in circumstances where JSON.stringify() may not be much help. My only concern here is whether or not we should display a string in double-quotes, as to coincide with JSON.stringify() behavior.
    • If value is undefined or null, return '[undefined]' or '[null]', respectively. It's possible to have strings with these same values, which may lead to confusion, but this is an edge case.
    • If value is not an Object, Function or Array, return result of value.toString().
    • If value is an empty Object, Function or Array, return result of function emptyRepresentation().
    • If value has properties, call exports.canonicalize() on it, then return result of JSON.stringify().
  • Modified exports.canonicalize() to support the above strategy, with a couple slight differences. The strategy is as follows. If the value...
    • has already been seen, return string '[Circular]'
    • is undefined, return string '[undefined]'
    • is null, return value null. This differs from the stringify() behavior, as to provide a valid JSON representation.
    • is a primitive, return the value
    • is not a primitive or an Array, Object, or Function, return the value of the Thing's toString() method
    • is a non-empty Array, Object, or Function, return the result of calling this function again.
    • is an empty Array, Object, or Function, return the result of calling emptyRepresentation()
  • Added many tests

Before:

      AssertionError: expected { Object (path, $explicit_fields, ...) } to deeply equal { Object ($accessed, $explicit_fields, ...) }
      + expected - actual

After:

      AssertionError: expected { Object (path, $explicit_fields, ...) } to deeply equal { Object ($accessed, $explicit_fields, ...) }
      + expected - actual

       {
      +  "$accessed": {}
      -  "$accessed": "Fri Sep 12 2014 23:00:10 GMT-0700 (PDT)"
         "$explicit_fields": []
      +  "$registered": {}
      -  "$registered": "Fri Sep 12 2014 23:00:10 GMT-0700 (PDT)"
         "author": "Christopher Hiller <chiller@badwing.com>"
         "author.email": "chiller@badwing.com"
         "author.name": "Christopher Hiller"
         "authors": [
           "Christopher Hiller <chiller@badwing.com>"
         ]
      -  "description": "[undefined]"
         "license": "Copyright 2014 Christopher Hiller <chiller@badwing.com>"
         "name": "riffraff"
         "path": "/Volumes/samara/projects/boneskull/test"
      -  "repository": "[undefined]"
         "repository.type": "git"
      -  "repository.url": "[undefined]"
      -  "version": "[undefined]"
       }

@boneskull
Copy link
Member Author

Also, I'm unsure if this breaks anything. 😄

@boneskull
Copy link
Member Author

Updated with some fixes + squished

@boneskull boneskull added the status: waiting for author waiting on response from OP - more information needed label Oct 17, 2014
@dasilvacontin
Copy link
Contributor

I would maybe add some asserts for testing if circular refs are handled at deeper levels.

Everything else LGTM. 👍

@bajtos
Copy link

bajtos commented Nov 11, 2014

This is awesome improvement!

      +  "$accessed": {}
      -  "$accessed": "Fri Sep 12 2014 23:00:10 GMT-0700 (PDT)"

Is there an (easy) way how to detect special object types like Date and return a different empty value in such case? I find the Date value {} confusing. Is it null, undefined, new Date() or what?

"repository": "[undefined]"

Is there a way how render this without the enclosing double quotes? If I understand the current implementation correctly, it renders undefined and '[undefined]' as the same text.

expect({ foo: undefined }).to.eql({ foo: '[undefined]' });

Output based on my understanding (I did not run the code):

     + expected - actual

       {
      -  "foo": "[undefined]"
      +  "foo": "[undefined]"
       }

IMHO, none of that is a blocker preventing this patch from being landed.

@dasilvacontin dasilvacontin mentioned this pull request Nov 11, 2014
@travisjeffery
Copy link
Contributor

+  "$accessed": {}
 -  "$accessed": "Fri Sep 12 2014 23:00:10 GMT-0700 (PDT)"

what is this saying? it was an empty object before and now it's a date?

@boneskull

@boneskull
Copy link
Member Author

Yes, sorry, this was unclear. It’s a diff of the diff output. :P

On November 11, 2014 at 13:37:24, Travis Jeffery (notifications@github.com) wrote:

  • "$accessed": {}
  • "$accessed": "Fri Sep 12 2014 23:00:10 GMT-0700 (PDT)"

what is this saying? it was an empty object before and now it's a date?

@boneskull


Reply to this email directly or view it on GitHub.

@travisjeffery
Copy link
Contributor

ok but $accessed is a date there there, right?

i think it'd probably be a good idea to wrap it with "[]", e.g.: "[Fri Sep 12 2014 23:00:10 GMT-0700 (PDT)]". as is done with undefined. and then we're consistently using [] to show that it's a value. or maybe some other symbol(s) to know that it's not really a string.

@dasilvacontin
Copy link
Contributor

ping @boneskull

@boneskull
Copy link
Member Author

Will do

On Mon, Dec 8, 2014 at 3:41 AM, David da Silva Contín <
notifications@github.com> wrote:

ping @boneskull https://github.com/boneskull


Reply to this email directly or view it on GitHub
#1357 (comment).

I think it's better, anyway.  I was becoming frustrated by seeing my `Date`s and `Function`s represented as objects within the diffs.  Even more frustrating is when the diff result *appears to be exactly the same* but the assertion failed anyway.  So this is my attempt at making the diffs more friendly and understandable.

Summary of changes:

- Added inner function `emptyRepresentation()`, which is called in the case that your `Object`, `Function` or `Array` has nothing "in" it.  It attempts to make a unique representation of that empty value.
- Added private API function `exports.type()`, which can be used to deduce the type of any given value.  *I exported this function mainly for testing purposes*
- Modified `exports.stringify()` to avoid `JSON.stringify()` in circumstances where `JSON.stringify()` may not be much help.  My only concern here is whether or not we should display a string in double-quotes, as to coincide with `JSON.stringify()` behavior.
  - If `value` is `undefined` or `null`, return `'[undefined]'` or `'[null]'`, respectively.   *It's possible to have strings with these same values, which may lead to confusion, but this is an edge case.*
  - If `value` is a `Date`, return ISO8601 representation w/ formatting
  - If `value` is not an `Object`, `Function` or `Array`, return result of `value.toString()`.
  - If `value` is an *empty* `Object`, `Function` or `Array`, return result of function `emptyRepresentation()`.
  - If `value` has properties, call `exports.canonicalize()` on it, then return result of `JSON.stringify()`.
- Modified `exports.canonicalize()` to support the above strategy, with a couple slight differences.  The strategy is as follows.  If the value...
  - has already been seen, return string `'[Circular]'`
  - is `undefined`, return string `'[undefined]'`
  - is `null`, return value `null`.  This differs from the `stringify()` behavior, as to provide a valid JSON representation.
  - is some other primitive, return the value
  - is a Date, return wrapped ISO8601 representation
  - is not a primitive or an `Array`, `Object`, or `Function`, return the value of the Thing's `toString()` method
  - is a non-empty `Array`, `Object`, or `Function`, return the result of calling this function again.
  - is an empty `Array`, `Object`, or `Function`, return the result of calling `emptyRepresentation()`
- Added many tests
@boneskull
Copy link
Member Author

Alright, I integrated this with the changes for Buffer diffs, rebased, and changed how dates are displayed.

@boneskull
Copy link
Member Author

@bajtos

Is there a way how render this without the enclosing double quotes? If I understand the current implementation correctly, it renders undefined and '[undefined]' as the same text.

Maybe... If you have an object (meaning something with properties), it runs through JSON.stringify(). Of course, undefined values are stripped from JSON.stringify(). A custom replacer might work.

boneskull pushed a commit that referenced this pull request Dec 11, 2014
utils.js: significant modification of diff behavior
@boneskull boneskull merged commit 67a73e4 into mochajs:master Dec 11, 2014
@boneskull boneskull deleted the diffs branch June 9, 2016 03:46
@boneskull boneskull removed the status: waiting for author waiting on response from OP - more information needed label Dec 12, 2017
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

4 participants