-
-
Notifications
You must be signed in to change notification settings - Fork 652
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
Remove special edge cases from triangle exercise #1898
Conversation
Dear bobtfishThank you for contributing to the Go track on Exercism! 💙
Dear Reviewer/Maintainer
Automated comment created by PR Commenter 🤖. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with the changes except the conversion of floats to ints. After removing the Inf
and NaN
cases, I don't think having floats or ints makes a big difference, and the tests in the specifications do have tests with floats. We would to go against specs to have ints and I don't think it's worth it.
@junedev WDYT?
Unfortunately, I realized it is not quite as easy as this PR. This exercise is part of the problem spec repo and the test cases there explicitly include floating point calculations. We could still mark all the floating point test cases as excluded in the tests.toml file but this raises the question why no other track seems to have concerns regarding floating point comparison in this exercise. 🤔 I would like to quickly consult with other maintainers before this gets merged. |
@andrerfcsantos While I also prefer to stick to problem spec, the person in #1448 raises a valid point there. I would like to check whether there was some prior discussion on this. |
Fair enough. But I don't think there's a problem in keeping floats from our side. A simple comparison like |
@andrerfcsantos @bobtfish Thanks for your patience while I tried to make up my mind about this. I managed to dig out the issue in the problem spec repo that discusses this. exercism/problem-specifications#798 Their conclusion was it is ok to assume that the exercise will not cover cases where the computational precision is a problem. So I agree with @andrerfcsantos that we should keep the floating point version and just remove the init part about Inf etc. @bobtfish I am really sorry I didn't properly think this through before labeling the issue as ready to be worked on, thus causing unnecessary work. |
No biggie :) |
7003cee
to
d1aec4c
Compare
d1aec4c
to
ca13e54
Compare
Thanks for this! |
Fixes #1477