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

Fixes Deep Compare #35

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Fixes Deep Compare #35

wants to merge 3 commits into from

Conversation

azachar
Copy link

@azachar azachar commented May 7, 2021

Resolves #34

Plus I refactored in the separate commit tests, to be more resilient to similar errors.

Copy link

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Cool, looks reasonable to me overall! Just one thing I found and think should be fixed before merge: the && actualValue === undefined not being tested. Thanks for sending! 🚀

return;
}

assert.equal(obj[key], content[key]);
if (Array.isArray(content[key]) && !content[key].length && actualValue === undefined) {

Choose a reason for hiding this comment

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

&& actualValue === undefined

No tests fail if this is removed - both here and later on. Either there's some missing unit test to be added or these checks aren't necessary and can be removed. Either way, could you check and make the right change please?

@@ -25,11 +25,11 @@ describe('yeoman-assert', () => {

it('reject a file that does not exist', () => {
assert.throws(yoAssert.file.bind(yoAssert, 'etherealTestFile'));
});
}, /.*AssertionError.*/);

Choose a reason for hiding this comment

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

Suggestion: I'd revert these. I don't plan on blocking on this, but just stating a preference: I don't think these are worth adding. Nothing prevents the comments from getting out of date. And we already know some error is being thrown. If we really wanted to assert on which error that is, we can always check its class/constructor/name.

Totally up to you though, feel free to ignore this suggestion. 🙂

@JoshuaKGoldberg
Copy link

Btw, in case you're wondering why this took almost four years to review: yeoman/yeoman#1779. 🎩

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

Successfully merging this pull request may close these issues.

objectContent throws "Cannot read property 'xxx' of undefined" on mismatched nested objects
2 participants