-
-
Notifications
You must be signed in to change notification settings - Fork 105
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
Add Map predicate #36
Conversation
source/lib/predicates/map.ts
Outdated
*/ | ||
hasKeys(...keys: any[]) { | ||
return this.addValidator({ | ||
message: map => `Expected Map to have all keys of \`${JSON.stringify(keys)}\`, got \`${JSON.stringify(Array.from(map.keys()))}\``, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about stringifying only 3 items, for example? This is to prevent error message being really long, because the whole array and map get serialized. So if keys.length > 3
, show all keys of [1, 2, 3, ...]
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I wasn't sure about the error message either. Maybe we should be more specific and show which key could not be found? Because if we only show the first 3 items, the message could look like
Expected Map to have all keys of
[1, 2, 3, ...]
, got[1, 2, 3, ...]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great suggestion. Expected Map to have keys [1, 2, 3]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Expected Map to have keys [1, 2, 3]
?
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm looking into this and what we didn't consider was that testing this could cause a performance issue on very large datasets. Not sure if we should take this into account?
What I mean is that .every()
exits early when one of the items don't match. Determining which keys are missing in the map requires us to iterate over the entire array.
So either we go with that approach, or we take a different one. We could also exit early and just show the error message
Expected Map to have key
1
The downside is that maybe, key 2
is missing as well, but because we exited early (on the first mismatch), we don't know for sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also be fine with showing the first 3-5 to be honest. But no strong opinion on the amount.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. Let’s show the first 5.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if map's values aren't just numbers? Error message will be huge in that case. Not sure yet what's the best solution here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I totally agree, I just think this would be very difficult to do well with complex datastructures. Maybe we can just go with the showing the first 5 and improve later on, gather some feedback from users etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Let's do it simple for now and we can use some more elaborate heuristics when we get some user feedback.
Updated the methods! |
Looks great, as always! :) |
Implemented everything regarding
Map
.