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

Bug fix for the last test of Allergies #209

Closed
wants to merge 1 commit into from

Conversation

k0pernicus
Copy link

The last test must not contains false positives, but 509 is greater than 255 so expected must contain Allergen::Peanuts.

The last test must not contains false positives, but 509 is greater than 255 so `expected` must contain `Allergen::Peanuts`.
@petertseng
Copy link
Member

petertseng commented Sep 21, 2016

I think the test is correct as-is - 509 & 255 is 253, so the result for 509 should be the same as the result for 253, and the result for 253 does not contain Peanuts

Edit: correction in my comment: 253 instead of 252.

@petertseng
Copy link
Member

This test is present in the Rust track because it is present in the common test data at https://github.com/exercism/x-common/blob/master/exercises/allergies/canonical-data.json - if we wish to change it, it is best to change it there so that all tracks can update their tests

A discussion for numbers > 255 is at exercism/problem-specifications#224

@k0pernicus
Copy link
Author

Ok so write directly in the README file that if the numbers is greater than 255, you have to make a & with it?

I think it exists a better solution to this end, returning a Result type creating the structure Allergies.
This type can return the Self object for Allergies if the uid given as parameter is correct - else an Error type and we can't continue after that?

@petertseng
Copy link
Member

Yes, we should discuss at exercism/problem-specifications#224 whether the correct solution is to & 255 or say that numbers greater than 255 are errors.

@IanWhitney
Copy link
Contributor

I suggest we close this until exercism/problem-specifications#224 is resolved

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.

None yet

3 participants