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

Additional object type overrides #423

Merged
merged 17 commits into from Mar 29, 2018

Conversation

Projects
None yet
3 participants
@alexjeffburke
Collaborator

alexjeffburke commented Oct 15, 2017

Hey,

The changes in this commit are a sort of minimal set of overrides allowing the Map type to share inspect and diff code with the base object plugin. If you want to see how they are used, I split out the Map code itself into a separate repo https://github.com/alexjeffburke/unexpected-map.

My hope is what's here strikes a decent balance between re-use while avoiding invasive changes to other areas that might require other approaches (i.e. "to satisfy"). I'm more than willing to iterate on this and the main reason for sharing it now is to get agreement on the approach and early feedback.

Look forward to hearing your thoughts!

@papandreou

This looks really good so far! Here are my preliminary comments.

I'm assuming there'll be more later -- at the very least <object> to [exhaustively] satisfy <object> should also be updated to support the new hooks, right?

Show outdated Hide outdated lib/types.js
Show outdated Hide outdated lib/types.js
Show outdated Hide outdated lib/types.js
@sunesimonsen

Great work. I added some comments. As @papandreou to satisfy should use the same hooks.

Show outdated Hide outdated lib/styles.js
Show outdated Hide outdated lib/types.js
Show outdated Hide outdated lib/types.js
Show outdated Hide outdated lib/types.js
Show outdated Hide outdated test/types/object-type.spec.js
@alexjeffburke

This comment has been minimized.

Show comment
Hide comment
@alexjeffburke

alexjeffburke Oct 24, 2017

Collaborator

@sunesimonsen @papandreou I've now converted "to satisfy" via the use has a hasKey(). In actual fact I'd already written that code at the weekend when I added tests of similar() :)

Collaborator

alexjeffburke commented Oct 24, 2017

@sunesimonsen @papandreou I've now converted "to satisfy" via the use has a hasKey(). In actual fact I'd already written that code at the weekend when I added tests of similar() :)

@papandreou

This comment has been minimized.

Show comment
Hide comment
@papandreou

papandreou Oct 24, 2017

Member

Remember the array-like type and <array-like> to satisfy <array-like> :)

Member

papandreou commented Oct 24, 2017

Remember the array-like type and <array-like> to satisfy <array-like> :)

@alexjeffburke

This comment has been minimized.

Show comment
Hide comment
@alexjeffburke

alexjeffburke Oct 24, 2017

Collaborator

@papandreou haha yep, takin it one step at a time :p

Collaborator

alexjeffburke commented Oct 24, 2017

@papandreou haha yep, takin it one step at a time :p

@alexjeffburke

This comment has been minimized.

Show comment
Hide comment
@alexjeffburke

alexjeffburke Oct 29, 2017

Collaborator

@papandreou Think I just got the support working :)

Assuming CI comes back clean I think it is now safe to say array-like now uses valueKey().

Collaborator

alexjeffburke commented Oct 29, 2017

@papandreou Think I just got the support working :)

Assuming CI comes back clean I think it is now safe to say array-like now uses valueKey().

@papandreou

This comment has been minimized.

Show comment
Hide comment
@papandreou

papandreou Jan 30, 2018

Member

Talked to @albertfdp about using these hooks to support the https://facebook.github.io/immutable-js/ types without converting the entire structures being compared to JavaScript objects first. Instead the Immutable types could just inherit from the Unexpected ones and use getKeys, valueForKey overrides to make them work with the existing assertion/diffing code.

In other words, it would be really great to get this landed :)

Member

papandreou commented Jan 30, 2018

Talked to @albertfdp about using these hooks to support the https://facebook.github.io/immutable-js/ types without converting the entire structures being compared to JavaScript objects first. Instead the Immutable types could just inherit from the Unexpected ones and use getKeys, valueForKey overrides to make them work with the existing assertion/diffing code.

In other words, it would be really great to get this landed :)

@sunesimonsen sunesimonsen changed the base branch from master to alexjeffburke/additional-object-type-hooks Mar 4, 2018

@sunesimonsen sunesimonsen changed the base branch from alexjeffburke/additional-object-type-hooks to master Mar 4, 2018

@sunesimonsen

This comment has been minimized.

Show comment
Hide comment
@sunesimonsen

sunesimonsen Mar 4, 2018

Member

I opened #438 and is trying to rebase and see if we can get the branch in a mergeable state.

Member

sunesimonsen commented Mar 4, 2018

I opened #438 and is trying to rebase and see if we can get the branch in a mergeable state.

alexjeffburke added some commits Feb 18, 2018

Add required object type hooks to support Map plugin.
Provide support for overriding the how properties are drawn in the
object methods - use the hook and override it to use "[...]" when
outputting elements of the Map.

* allow changing the method used to calculate the uniqueKeys
* allow specifying the method used to retrieve the valueForKey
* attach the keyComparator to the object type to allow reuse
Ensure the keyComparator defaults to undefined.
This fixes Unexpected execution when built with es5-shim and thus
execution in the browser since null is disallowed.
Show outdated Hide outdated lib/types.js
Show outdated Hide outdated lib/types.js
Show outdated Hide outdated lib/types.js
@sunesimonsen

You done a fantastic job here! 💯 I added some minor nits, but feel free to fix and merge or ignore them.

@alexjeffburke

This comment has been minimized.

Show comment
Hide comment
@alexjeffburke

alexjeffburke Mar 25, 2018

Collaborator

@sunesimonsen thanks for the review and the kind words :)

I confess to being a little unsure about the API change you wanted, but I've just pushed up a commit which changes all the type property() calls to take cloned output - that result is then intermixed with the other output as appropriate by the caller. If that's not quite what you were looking for just let me know, but I will say I think it looks a lot cleaner now.

I have WIP code for a Map plugin somewhere here and I'd still like to check it works against this version of the hooks after all the changes and rebasing before any release, but I'm feeling pretty confident about what's here at the moment. Let me know what you and @papandreou think.

Collaborator

alexjeffburke commented Mar 25, 2018

@sunesimonsen thanks for the review and the kind words :)

I confess to being a little unsure about the API change you wanted, but I've just pushed up a commit which changes all the type property() calls to take cloned output - that result is then intermixed with the other output as appropriate by the caller. If that's not quite what you were looking for just let me know, but I will say I think it looks a lot cleaner now.

I have WIP code for a Map plugin somewhere here and I'd still like to check it works against this version of the hooks after all the changes and rebasing before any release, but I'm feeling pretty confident about what's here at the moment. Let me know what you and @papandreou think.

@alexjeffburke

This comment has been minimized.

Show comment
Hide comment
@alexjeffburke

alexjeffburke Mar 25, 2018

Collaborator

Hmm seems like I may have a missed a couple - I’ll do another pass after giving my brain a little break.

Collaborator

alexjeffburke commented Mar 25, 2018

Hmm seems like I may have a missed a couple - I’ll do another pass after giving my brain a little break.

alexjeffburke added some commits Mar 25, 2018

Pass over types for cloned output to type.property() calls.
This commit also aligns the array-like diff drawing code much more
closely with its counterpart in the "to satisfy" assertion.
@alexjeffburke

This comment has been minimized.

Show comment
Hide comment
@alexjeffburke

alexjeffburke Mar 26, 2018

Collaborator

@sunesimonsen @papandreou I've just pushed up what I think may be the final changes to address the review comments. All the calls into property() methods on types are given output.clone() - as an additional consequence, the drawing parts of the diff code for array-like types has begun to look a lot closer to the code that's within the "to satisfy" assertion. Let my know what you think.

Collaborator

alexjeffburke commented Mar 26, 2018

@sunesimonsen @papandreou I've just pushed up what I think may be the final changes to address the review comments. All the calls into property() methods on types are given output.clone() - as an additional consequence, the drawing parts of the diff code for array-like types has begun to look a lot closer to the code that's within the "to satisfy" assertion. Let my know what you think.

@alexjeffburke alexjeffburke changed the title from WIP - additional object type overrides to Additional object type overrides Mar 26, 2018

@alexjeffburke

This comment has been minimized.

Show comment
Hide comment
@alexjeffburke

alexjeffburke Mar 26, 2018

Collaborator

The unexpected-map plugin worked against this version of Unexpected. Have asked @papandreou to look over one of the last refactoring in the diff but otherwise I think this is ready :shipit:

Collaborator

alexjeffburke commented Mar 26, 2018

The unexpected-map plugin worked against this version of Unexpected. Have asked @papandreou to look over one of the last refactoring in the diff but otherwise I think this is ready :shipit:

@papandreou

Yay! Some minor stuff, but looks great!

Show outdated Hide outdated lib/assertions.js
.caught(() =>
expect.promise.settle(keyPromises).then(() => {
.caught(() => {
let i = 0;

This comment has been minimized.

@papandreou

papandreou Mar 26, 2018

Member

I guess we can move the let i = 0s into the for(s below now that we're transpiling?

@papandreou

papandreou Mar 26, 2018

Member

I guess we can move the let i = 0s into the for(s below now that we're transpiling?

@@ -27,6 +27,26 @@ const utils = (module.exports = {
return a === b;
}),
duplicateArrayLikeUsingType(obj, type) {

This comment has been minimized.

@papandreou

papandreou Mar 26, 2018

Member

Hmm, shouldn't this function use type.getKeys instead of Object.keys and Object.getOwnPropertySymbols?

@papandreou

papandreou Mar 26, 2018

Member

Hmm, shouldn't this function use type.getKeys instead of Object.keys and Object.getOwnPropertySymbols?

This comment has been minimized.

@alexjeffburke

alexjeffburke Mar 28, 2018

Collaborator

@papandreou I've been thinking about this on and off for a few days now.. part of me think running through getKeys() makes sense, but I'm also quite hesitant - IIRC at the point where the arrays are compared they it never previously ran through getKeys and something vague keeps nagging me I did it this way on purpose.

@alexjeffburke

alexjeffburke Mar 28, 2018

Collaborator

@papandreou I've been thinking about this on and off for a few days now.. part of me think running through getKeys() makes sense, but I'm also quite hesitant - IIRC at the point where the arrays are compared they it never previously ran through getKeys and something vague keeps nagging me I did it this way on purpose.

This comment has been minimized.

@papandreou

papandreou Mar 28, 2018

Member

Since duplicateArrayLikeUsingType is being used in <array-like> to satisfy <array-like>, I think it might break if you have a subtype of array-like that has a custom getKeys and non-enumerable properties that are included by the getKeys. In that case I think you'll get an incomplete diff rendered if getKeys is not consulted.

Something like (untested):

expect.addType({
  name: 'foo',
  base: 'array-like',
  identify(obj) {
   return obj && obj._isFoo;
  },
  getKeys(obj) {
    return [...Object.keys(obj), 'bar'];
  }
});

const foo1 = ['hey', 'there'];
const foo2 = ['hey', 'there'];
Object.defineProperty(foo1, 'bar', {
  value: 123,
  enumerable: false
});
Object.defineProperty(foo2, 'bar', {
  value: 456,
  enumerable: false
});

expect(foo1, 'to satisfy', foo2); // will fail but the diff won't mention the bar attributes
@papandreou

papandreou Mar 28, 2018

Member

Since duplicateArrayLikeUsingType is being used in <array-like> to satisfy <array-like>, I think it might break if you have a subtype of array-like that has a custom getKeys and non-enumerable properties that are included by the getKeys. In that case I think you'll get an incomplete diff rendered if getKeys is not consulted.

Something like (untested):

expect.addType({
  name: 'foo',
  base: 'array-like',
  identify(obj) {
   return obj && obj._isFoo;
  },
  getKeys(obj) {
    return [...Object.keys(obj), 'bar'];
  }
});

const foo1 = ['hey', 'there'];
const foo2 = ['hey', 'there'];
Object.defineProperty(foo1, 'bar', {
  value: 123,
  enumerable: false
});
Object.defineProperty(foo2, 'bar', {
  value: 456,
  enumerable: false
});

expect(foo1, 'to satisfy', foo2); // will fail but the diff won't mention the bar attributes

This comment has been minimized.

@alexjeffburke

alexjeffburke Mar 28, 2018

Collaborator

@papandreou in that case this might actually be a pre-existing niggle - note that the unprocessed arrays were passed into arrayChanges previously:
https://github.com/unexpectedjs/unexpected/pull/423/files#diff-0a56a2b979f6080486ec1909508cda1aR1539

I had thought there was some other mechanism that factored getKeys into the mix but perhaps not. Will look into this and try to reach a conclusion one way or the other.

@alexjeffburke

alexjeffburke Mar 28, 2018

Collaborator

@papandreou in that case this might actually be a pre-existing niggle - note that the unprocessed arrays were passed into arrayChanges previously:
https://github.com/unexpectedjs/unexpected/pull/423/files#diff-0a56a2b979f6080486ec1909508cda1aR1539

I had thought there was some other mechanism that factored getKeys into the mix but perhaps not. Will look into this and try to reach a conclusion one way or the other.

@@ -1772,18 +1783,16 @@ module.exports = expect => {
keys.forEach((key, index) => {
promiseByKey[key] = expect.promise(() => {
const valueKeyType = expect.findTypeOf(value[key]);
const subjectKey = subjectType.valueForKey(subject, key);
const valueKey = valueType.valueForKey(value, key);

This comment has been minimized.

@papandreou

papandreou Mar 26, 2018

Member

IMO the subjectKey and valueKey variable names makes it sound like it's keys from the objects, but it's actually values.

@papandreou

papandreou Mar 26, 2018

Member

IMO the subjectKey and valueKey variable names makes it sound like it's keys from the objects, but it's actually values.

clonedExpect.addType({
name: 'xuuq',
base: 'object',
identify: function(obj) {

This comment has been minimized.

@papandreou

papandreou Mar 26, 2018

Member

identify(obj) { :)

@papandreou

papandreou Mar 26, 2018

Member

identify(obj) { :)

identify: function(obj) {
return obj && typeof 'object' && obj.quux === 'xuuq';
},
property: function(output, key, inspectedValue) {

This comment has been minimized.

@papandreou

papandreou Mar 26, 2018

Member

property(output, key, inspectedValue) {

@papandreou

papandreou Mar 26, 2018

Member

property(output, key, inspectedValue) {

});
expect(
function() {

This comment has been minimized.

@papandreou

papandreou Mar 26, 2018

Member

Arrow fn :)

@papandreou

papandreou Mar 26, 2018

Member

Arrow fn :)

base: 'array-like',
identify: Array.isArray,
numericalPropertiesOnly: false,
hasKey: function(obj, key) {

This comment has been minimized.

@papandreou

papandreou Mar 26, 2018

Member

hasKey(obj, key) {

@papandreou

papandreou Mar 26, 2018

Member

hasKey(obj, key) {

@alexjeffburke

This comment has been minimized.

Show comment
Hide comment
@alexjeffburke

alexjeffburke Mar 28, 2018

Collaborator

I've responded to all the feedback bar the point about duplicateArrayLikeUsingType() which I'll reply to above.

I've tracked down the last missed valueForKey case plus converted a couple of the more specific assertions that key into objects/array-like. Also, want to respond to a couple of the other points:

  • naming
    I agree that some of the names are perhaps misleading - currently they are at least consistent so I'd suggest we land this working as-is and I'll look at doing a cleanup pass
  • ES5 vs ES6 in test files
    The tests are predominantly ES5 - keeping it that way actually allows the tests to be run directly in older browsers, something I have a WIP for so I'd prefer not to add any complexity for now.

Thanks for all the review time and I'm getting really excited to see this land :D :shipit:

Collaborator

alexjeffburke commented Mar 28, 2018

I've responded to all the feedback bar the point about duplicateArrayLikeUsingType() which I'll reply to above.

I've tracked down the last missed valueForKey case plus converted a couple of the more specific assertions that key into objects/array-like. Also, want to respond to a couple of the other points:

  • naming
    I agree that some of the names are perhaps misleading - currently they are at least consistent so I'd suggest we land this working as-is and I'll look at doing a cleanup pass
  • ES5 vs ES6 in test files
    The tests are predominantly ES5 - keeping it that way actually allows the tests to be run directly in older browsers, something I have a WIP for so I'd prefer not to add any complexity for now.

Thanks for all the review time and I'm getting really excited to see this land :D :shipit:

@alexjeffburke alexjeffburke merged commit a92f38f into unexpectedjs:master Mar 29, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.03%) to 98.184%
Details
@papandreou

This comment has been minimized.

Show comment
Hide comment
@papandreou

papandreou Mar 29, 2018

Member

🎉

Member

papandreou commented Mar 29, 2018

🎉

@alexjeffburke alexjeffburke deleted the alexjeffburke:additional-object-type-hooks branch May 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment