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

Fix some issues with value equality #32

Merged
merged 4 commits into from
Apr 27, 2021
Merged

Fix some issues with value equality #32

merged 4 commits into from
Apr 27, 2021

Conversation

Avaq
Copy link
Member

@Avaq Avaq commented Apr 19, 2021

While updating to 5.0 I found my tests' assertions started failing for some of the Json values. These fixes should address that, and similar, problems.

This fixes an issue where two equivalent instances of Json are not
considered equal, because after JSON.stringifying the input object, the
key order is taken into account when considering equality.
@Avaq Avaq requested a review from dicearr April 19, 2021 11:07
@Avaq Avaq self-assigned this Apr 19, 2021
});

test ('Json', () => {
eq (lib.Response.is (lib.Json (null)), true);
eq (lib.Json (null), lib.Json (null));
eq (lib.Json ({a: 1, b: 2}), lib.Json ({b: 2, a: 1}));
Copy link
Member Author

Choose a reason for hiding this comment

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

This assertion would fail, previously, because of the key order.

@codecov
Copy link

codecov bot commented Apr 19, 2021

Codecov Report

Merging #32 (7fcb535) into master (7375125) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 7fcb535 differs from pull request most recent head 79cdf51. Consider uploading reports for the commit 79cdf51 to get more accurate results
Impacted file tree graph

@@            Coverage Diff            @@
##            master       #32   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines          461       478   +17     
=========================================
+ Hits           461       478   +17     
Impacted Files Coverage Δ
index.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7375125...79cdf51. Read the comment docs.

index.js Outdated Show resolved Hide resolved
This function can be (and will be) used more generally to match against
a subset of tags, with each of the other tags resulting in the provided
default value.
@@ -244,16 +262,15 @@ export const Text = value => Response.Respond (
Body.Send (value),
);

//# Json :: Object -> Response a b
//# Json :: JsonValue -> Response a b
Copy link
Contributor

Choose a reason for hiding this comment

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

Does JsonValue mean something like any JSON-serializable data?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. Not all values of type Object are valid input here.

index.js Outdated
Comment on lines 229 to 235
return this.cata ({
None: _ => cataBool ({None: _ => true}) (other),
Send: a => cataBool ({Send: b => Z.equals (a, b)}) (other),
Json: a => cataBool ({Json: b => Z.equals (a, b)}) (other),
Stream: a => cataBool ({Stream: b => a === b}) (other),
Render: (...a) => cataBool ({Render: (...b) => Z.equals (a, b)}) (other),
});
Copy link
Contributor

Choose a reason for hiding this comment

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

You could avoid repeating (other):

return this.cata ({
  // ...
}) (other);

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, yes. Nifty. :)

This commit fixes an issue where two equivalent values of type Stream
are not considered equal. This happens because the inner value of a
Stream (a Future) does not implement Setoid. We fix this by using
reference equality.
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.

3 participants