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

Add implementation of includes and unit tests #111

Merged
merged 6 commits into from
Oct 14, 2015

Conversation

cflynn07
Copy link
Contributor

Implementation for #109

Went a little further than the functionality in the proposed example of 109 and added an optional "fromIndex" argument to limit search space of array (functionality as defined in ES7 proposal).

@cflynn07
Copy link
Contributor Author

One note: my unit tests wont hit https://github.com/cflynn07/101/blob/109-add-includes/includes.js#L28

Want to monkey patch Array.prototype.includes in the tests?

@bkendall
Copy link
Collaborator

Want to monkey patch Array.prototype.includes in the tests?

Array.prototype.includes = sinon.stub().returns(42);
// create a new array
// search said array
// assert 42 was returned (whatever .includes returns this should return)
// delete (.restore() may not do it) the stubbed includes method

Typing that on an iPad is tough. But, that's what I would do in a test. Doesn't need to actually search it, just return whatever includes would procide. It's weird adding functions to global namespace, but you can give it a try!

while (k < len) {
currentElement = O[k];
if (searchElement === currentElement ||
(searchElement !== searchElement && currentElement !== currentElement)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

My brain is struggling with this second half of the if statement (it is also before 6am...) why would you check that both aren't equal to themselves? Feels like a very JavaScript thing to do though...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a great question. This code is copy/pasted from MDN documentation. I'm not sure what the purpose of that is. I asked a question here:
http://stackoverflow.com/questions/33068441/es7-array-prototype-includes-polyfill-double-equality-comparison

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some more information here: https://tc39.github.io/Array.prototype.includes/
If SameValueZero(searchElement, elementK) is true, return true.

http://www.ecma-international.org/ecma-262/6.0/index.html#sec-samevaluezero

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct identification of values such as NaN and -0 && 0 as equal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will add a test for those conditions

@tjmehta
Copy link
Owner

tjmehta commented Oct 11, 2015

LGTM, just missing 100% test-coverage

@cflynn07
Copy link
Contributor Author

Will add documentation tomorrow

@tjmehta
Copy link
Owner

tjmehta commented Oct 13, 2015

How about some partial functionality goodness so we can support the example in #109?

@cflynn07
Copy link
Contributor Author

That's present in this PR. I'll add a doc example of it

Sent from my iPhone

On Oct 12, 2015, at 5:36 PM, Tejesh Mehta notifications@github.com wrote:

How about some partial functionality goodness so we can support the example in #109?


Reply to this email directly or view it on GitHub.

@cflynn07
Copy link
Contributor Author

@tjmehta added example of support of functionality in #109 in README

@tjmehta
Copy link
Owner

tjmehta commented Oct 14, 2015

👍

tjmehta added a commit that referenced this pull request Oct 14, 2015
Add implementation of includes and unit tests
@tjmehta tjmehta merged commit e372a63 into tjmehta:master Oct 14, 2015
@tjmehta
Copy link
Owner

tjmehta commented Oct 14, 2015

101@v1.2.0

@cflynn07 cflynn07 deleted the 109-add-includes branch December 31, 2015 17:35
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