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

Include non-enumerable properties #482

Merged
merged 10 commits into from Jun 29, 2018
Merged

Include non-enumerable properties #482

merged 10 commits into from Jun 29, 2018

Conversation

@alexjeffburke
Copy link
Member

@alexjeffburke alexjeffburke commented Jun 2, 2018

This branch adds support for including all properties regardless of enumerability for object comparisons. This is definitely further than a prototype but I imagine will want a thorough review both on the code itself but also 'philosophically'. Note some interesting cleanups/simplifications in a few places were possible around key handling.

Move checking object equality into a separate common function
utils.checkObjectEqualityUsingType(). Use this within the base object
type but also when comparing Error keys - since this function accepts
the type, in the case of the Error type it ensures equality is
evaluated using the subtype getKeys() thus addressing the mismatching
keys issue.
Account for the addition of an "isOperational" property that is added
for any error that passes through the bluebird promise implementation.
Copy link
Member

@papandreou papandreou left a comment

I'm on board with this. Looks great and the approach seems solid!

const valueKey = valueType.valueForKey(value, key);
const valueKeyType = expect.findTypeOf(valueKey);
const isDefinedSubjectKey = typeof subjectKey !== 'undefined';
const isDefinedValueKey = typeof valueKey !== 'undefined';

This comment has been minimized.

@papandreou

papandreou Jun 11, 2018
Member

Shouldn't Key be replaced by Value in the above 5 variable names?

@sunesimonsen
Copy link
Member

@sunesimonsen sunesimonsen commented Jun 12, 2018

@alexjeffburke the Node 0.10 build has failed, is that something that is fixable?

const subjectKeys = subjectType.getKeys(subject);
const valueKeys = valueType.getKeys(value);

This comment has been minimized.

@sunesimonsen

sunesimonsen Jun 20, 2018
Member

Mhh I know this is my mistake, but I think things will be much more clear if we called subject actual and value expected in this method. We can always fix that later.

This comment has been minimized.

@sunesimonsen

sunesimonsen Jun 20, 2018
Member

or maybe subject and spec.

@sunesimonsen
Copy link
Member

@sunesimonsen sunesimonsen commented Jun 20, 2018

I tried to improve performance by using indexes and what not, without luck 👍 It is as fast as master. Node v0.10.48 does not fail locally - don't know what is going on with that.

It is only the failing tests that worries me right now, otherwise I would say 🚢

@sunesimonsen
Copy link
Member

@sunesimonsen sunesimonsen commented Jun 20, 2018

I can reproduce the failing tests, by merging to master locally and running make travis on Node v0.10.48.

@sunesimonsen
Copy link
Member

@sunesimonsen sunesimonsen commented Jun 20, 2018

If I blacklist arguments and type on the error types I'm left with this error:
screen shot 2018-06-20 at 22 13 23

@sunesimonsen
Copy link
Member

@sunesimonsen sunesimonsen commented Jun 20, 2018

Okay here is the plan. When on Node 0.10 blacklist arguments and type for Error and UnexpectedError. Change the test above to use list instead of 'a list' and close your eyes.

Then this should be good to merge.

Thanks for the hard work and sorry for the late review.

@alexjeffburke
Copy link
Member Author

@alexjeffburke alexjeffburke commented Jun 20, 2018

Thanks both for looking at this - I’ll try to find time to execute on the comments over the next few days. Look forward to getting this in too :)

@sunesimonsen
Copy link
Member

@sunesimonsen sunesimonsen commented Jun 21, 2018

👍 I think the change will only take you about 5 minutes as we already found the solution, so do worry that fixing this is a big task.

It is really great to see very deep improvements like this, well done 👌

@papandreou
Copy link
Member

@papandreou papandreou commented Jun 21, 2018

@alexjeffburke
Copy link
Member Author

@alexjeffburke alexjeffburke commented Jun 29, 2018

@papandreou I've folded your fixes into this branch - thank you and @sunesimonsen for tackling these last bits. With the tests all passing, how do you both feel if finally go ahead and merge? :D

@papandreou
Copy link
Member

@papandreou papandreou commented Jun 29, 2018

I guess that depends on whether we want to get it into a major release with those other things? Either way, I think it'd be fine to merge it and then create a legacy v10 branch to do releases from in the mean time.

@alexjeffburke alexjeffburke merged commit 4986d6a into unexpectedjs:master Jun 29, 2018
2 checks passed
2 checks passed
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls First build on fix/node0.10 at 98.139%
Details
@sunesimonsen
Copy link
Member

@sunesimonsen sunesimonsen commented Jun 29, 2018

@papandreou too late, but I'm not overly happy about master becoming a development branch for v11. What is breaking about this PR?

@sunesimonsen
Copy link
Member

@sunesimonsen sunesimonsen commented Jun 29, 2018

It must be some subtle breakage.

@alexjeffburke
Copy link
Member Author

@alexjeffburke alexjeffburke commented Jun 29, 2018

@sunesimonsen ah crap, sorry - it’s only if folks were relying on non-enumerates properties being ignored. In practice I really doubt this affects anyone, and if it does then there’s a chance it’s helpful to highlight this to them.

I’ve been 50-50 about just releasing this as another minor, which would also be equally fair - I’d love to see this in people’s hands of course :)

@papandreou
Copy link
Member

@papandreou papandreou commented Jun 29, 2018

@sunesimonsen, the fact that object to equal and to satisfy now consider non-enumerable properties as well is technically a breaking change. We don't know the severity of it, but I'd be surprised if this didn't cause someone's test suite to fail.

@sunesimonsen
Copy link
Member

@sunesimonsen sunesimonsen commented Jun 29, 2018

I think the major makes sense just out of principal. But I don't like changing our release and branching strategy for it. I guess we just need to get v11 in shape then 😊

@alexjeffburke
Copy link
Member Author

@alexjeffburke alexjeffburke commented Jun 29, 2018

Hahaha ok then I’ll take my part of the consequences of having lit a fire under v11 :p ..will give the branches you mentioned before another review @papandreou. cc @sunesimonsen

@sunesimonsen
Copy link
Member

@sunesimonsen sunesimonsen commented Jun 29, 2018

I think we should just aim for a small enhancement version and push larger features to v12.

@alexjeffburke alexjeffburke deleted the alexjeffburke:feature/includeNonEnumerableProperties branch Sep 1, 2018
sunesimonsen added a commit that referenced this pull request Sep 5, 2018
…EnumerableProperties"

This reverts commit 4986d6a, reversing
changes made to 5884c76.
sunesimonsen added a commit that referenced this pull request Sep 5, 2018
…cludeNonEnumerableProperties""

This reverts commit aeac1ef.
@sunesimonsen sunesimonsen mentioned this pull request Sep 5, 2018
25 of 25 tasks complete
@sunesimonsen
Copy link
Member

@sunesimonsen sunesimonsen commented Sep 5, 2018

Moved to #509.

sunesimonsen added a commit that referenced this pull request Sep 5, 2018
Revert "Merge pull request #482 from alexjeffburke/feature/includeNonEnumerableProperties"
sunesimonsen added a commit that referenced this pull request Sep 5, 2018
…cludeNonEnumerableProperties""

This reverts commit aeac1ef.
@sunesimonsen sunesimonsen added this to the v11 milestone Sep 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants