Skip to content
This repository has been archived by the owner on Aug 1, 2021. It is now read-only.

Extra tests for the 'binary' exercise. #121

Merged
merged 1 commit into from
Oct 26, 2015

Conversation

creature
Copy link
Contributor

cf. x-common issue #95.

I've expanded the JS test for the 'binary' exercise to include the same examples as the Ruby equivalent. The example solution has also been updated so it passes the new tests.

However, I'm not familiar enough with the Exercism progression to know if this is pitched at an appropriate level – that is, if students are expected to do string validation or understand the nuances of parseInt at this point. JavaScript's parseInt is rather more flexible than solutions in other languages, and doesn't make it easy to bail out on invalid input:

If parseInt encounters a character that is not a numeral in the specified radix, it ignores it and all succeeding characters and returns the integer value parsed up to that point... If the first character cannot be converted to a number, parseInt returns NaN.

If there's a different approach you'd like me to take with this, let me know and I'll adjust this PR to suit. :)

@kytrinyx
Copy link
Member

This looks good. I think the progression is still fine the way it is. @rchavarria do you want to have a look at this and merge if you're happy with it?

@rchavarria
Copy link
Contributor

It looks ok. In JavaScript, parseInt parses "10nope" as a valid number, but README file asks for handling invalid outputs. Even though parseInt returns a number, I agree it is an invalid output in this case.

I would merge this PR but it has conflicts. I think is because of file names. binary-test.spec.js file is now called binary.spec.js. Would you mind to resolve conflicts @creature? I don't know if it would be better to amend this PR or create a new one, it's up to you.

@kytrinyx
Copy link
Member

The conflict should resolve itself fairly easily if you rebase onto the most recent upstream master.

@kytrinyx
Copy link
Member

Also: you can replace a pull request by doing a git push -f to the branch.

…em-specifications#95

This expands the JavaScript test for the 'binary' exercise to include
the same examples as the Ruby equivalent. The example solution has also
been updated so it passes the new tests.
@creature
Copy link
Contributor Author

@rchavarria & @kytrinyx: I'm glad to hear that it's at an appropriate level. I've rebased and pushed, so it should merge cleanly now. I had to do that last week too, when underscores became dashes, so it wasn't a problem. :)

@kytrinyx
Copy link
Member

Sweet! Thanks so much.

kytrinyx added a commit that referenced this pull request Oct 26, 2015
Extra tests for the 'binary' exercise.
@kytrinyx kytrinyx merged commit 5dc983a into exercism:master Oct 26, 2015
@rchavarria
Copy link
Contributor

Thanks @creature

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

Successfully merging this pull request may close these issues.

3 participants