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

Tests for all-your-base look wrong #206

Closed
rmunn opened this issue Sep 3, 2016 · 4 comments · Fixed by #207
Closed

Tests for all-your-base look wrong #206

rmunn opened this issue Sep 3, 2016 · 4 comments · Fixed by #207

Comments

@rmunn
Copy link
Contributor

rmunn commented Sep 3, 2016

I just got the new all-your-base exercise for the first time. And as I was looking through the unit tests to see the spec I'm implementing against, I noticed that an empty list [] is considered to be invalid input, rather than 0. That's a debatable issue (and I would disagree), but that's a place where opinion can validly differ -- if you have a string with no digits in it at all, it's fair enough to consider that to be "not a number" rather than representing 0, so a list with no digits could also represent 0. It will make my code slightly less elegant than it could have been, but fair enough.

But then I saw the next test, where the input is [0] and the output is None. That one's not debatable; it's just wrong. An input of a single 0 digit should produce the number 0 in the output base, and should not be an error.

And the next two tests after that, where the input is either multiple zeroes ([0; 0; 0]) or contains a leading zero ([0; 6; 0]), also expect an output of None. And here also, I believe the tests are wrong, and the correct answer should be 0, and "60" in whatever the input base is (in that particular test it's 7, so 60 is 42 in decimal).

Looking at https://github.com/exercism/x-common/blob/master/all-your-base.json for a bit, I figured out that the tests were automatically converted to F#, and the null results in that JSON file were turned into None. But the comment at the top of that JSON file says that it's those are tests where the right behavior could vary by language, and it's up to each language track to determine what the right behavior should be. I.e., they say that the correct representation of zero might be an empty list, or a list with a single zero, or a list with multiple zeroes... and that's why all those tests have a defined result of null. But at least one of them should be possible; as it stands, the F# version of the all-your-base exercise does not allow zero to be represented at all.

Which, in a test that was named after Zero Wing, just seems... well, wrong somehow.

The tests should be fixed... for great justice.

@ErikSchierboom
Copy link
Member

Good point. What do you propose to be the correct handling of zeroes and such?

@rmunn
Copy link
Contributor Author

rmunn commented Sep 3, 2016

How are you gentlemen !!

I set up you the PR.

@rmunn
Copy link
Contributor Author

rmunn commented Sep 3, 2016

Couldn't resist. :-) But seriously, what I did in the PR is what I'd suggest: empty lists are considered errors (I changed my mind a bit after thinking about it, and decided that it should be like parsing a string. No digits in the string? Then you don't get a number at all. No digits in the list? Likewise, no number.) For any other leading zeroes, though, it should parse just like a string should: leading zeroes are ignored. (And leading zeroes should certainly NOT mean "parse this as octal"; that is C's second-biggest blunder. The biggest, of course, is getting involved in a land war in... no, wait, I mean nulls.)

@ErikSchierboom
Copy link
Member

Seems like very sensible fixes. I've commented on your PR.

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 a pull request may close this issue.

2 participants