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

Say README suggests to test -1 but the unit tests expect uint64 #837

Closed
shaleh opened this issue Aug 29, 2017 · 6 comments
Closed

Say README suggests to test -1 but the unit tests expect uint64 #837

shaleh opened this issue Aug 29, 2017 · 6 comments
Labels
bug Marks that something is broken and needs to be fixed.

Comments

@shaleh
Copy link

shaleh commented Aug 29, 2017

No description provided.

@shaleh
Copy link
Author

shaleh commented Aug 30, 2017

Further, the README says code should report any values that are out of range. But Say takes a uint64. So negatives are not possible. If the input is larger than max unsigned 64-bit the value will wrap before the function ever sees it. Input validation is a total red herring.

@robphoenix
Copy link
Contributor

Thanks @shaleh

@robphoenix robphoenix added discrepancy bug Marks that something is broken and needs to be fixed. and removed discrepancy labels Aug 30, 2017
@MatForsberg
Copy link
Contributor

I believe since the instructions say any number from 0 to 999,999,999,999 and an uint64's max value is 18,446,744,073,709,551,615 that the request to report values out range refers to number from 1,000,000,000,000 to 18,446,744,073,709,551,615. A negative number would however cause an error in go. In rewriting this README would it be best to clarify the out of range testing or to just remove it along with the suggestion to test with -1?

@shaleh
Copy link
Author

shaleh commented Sep 20, 2017

Actually,since this is one of the earlier problems perhaps switch back to a standard signed int would be the right thing to do? This way the user gets to implement input validation. The current tests validate that ANY number that a uint64 can hold will work. Which directly contradicts the README since -1 can't be an input and numbers bigger than 1,000,000,000,000 are explicitly checked and expected to work.

Otherwise, document that no validation needs to be done and expect anything up to math.MaxUint64.

@robphoenix
Copy link
Contributor

perhaps switch back to a standard signed int would be the right thing to do?

I agree with @shaleh here, I think say should use an int64. This would bring it in line with the canonical-data.json. I see no reason why this track should diverge from this. We should then update the test cases, and eventually implement a test generator, to reflect this.

shaleh pushed a commit to shaleh/exercism-go that referenced this issue Oct 5, 2017
tleen pushed a commit that referenced this issue Oct 10, 2017
@tleen
Copy link
Member

tleen commented Oct 10, 2017

Closed via 3e89b2c

@tleen tleen closed this as completed Oct 10, 2017
robphoenix pushed a commit that referenced this issue Oct 10, 2017
robphoenix pushed a commit to robphoenix/exercism-go that referenced this issue Oct 10, 2017
robphoenix pushed a commit to robphoenix/exercism-go that referenced this issue Oct 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Marks that something is broken and needs to be fixed.
Projects
None yet
Development

No branches or pull requests

4 participants