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

Support for deep `Map` equality with `asserts#equal` #3236

Merged
merged 5 commits into from Oct 30, 2019

Conversation

@jamesseanwright
Copy link
Contributor

jamesseanwright commented Oct 29, 2019

As per #3235, here is my fix for supporting for deeply comparing Map instances in the asserts module's equal function. Much like the conditional branches for Set comparisons, this logic has been inlined within equal itself.

I've also included a set of test cases (simple equality, nested Maps , arrays and objects as Map values etc.)

messages.push(c(`${createSign(result.type)}${result.value}`));
}
);
diffResult.forEach((result: DiffResult<string>): void => {

This comment has been minimized.

Copy link
@jamesseanwright

jamesseanwright Oct 29, 2019

Author Contributor

Strangely, this unrelated block seems to have been changed when formatting this module. Shall I revert this? I appreciate it's a bit misleading given it isn't directly related to my changes.

This comment has been minimized.

Copy link
@jamesseanwright

jamesseanwright Oct 29, 2019

Author Contributor

Disregard. This was a symptom of my using deno fmt directly slaps self on wrist.

@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Oct 29, 2019

CLA assistant check
All committers have signed the CLA.

@jamesseanwright

This comment has been minimized.

Copy link
Contributor Author

jamesseanwright commented Oct 29, 2019

Noticed the lint check is failing. I'll take a look shortly.

@jamesseanwright

This comment has been minimized.

Copy link
Contributor Author

jamesseanwright commented Oct 29, 2019

Noticed the lint check is failing. I'll take a look shortly.

Looks like I've fixed it.

@ry
ry approved these changes Oct 30, 2019
Copy link
Collaborator

ry left a comment

LGTM

@ry ry merged commit 64957d9 into denoland:master Oct 30, 2019
10 checks passed
10 checks passed
test macOS-10.14
Details
test_std macOS-10.14
Details
test windows-2019
Details
test_std windows-2019
Details
test ubuntu-16.04
Details
test_debug ubuntu-16.04
Details
test_std ubuntu-16.04
Details
bench ubuntu-16.04
Details
lint ubuntu-16.04
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.