resistor-color-trio: Add missing test case from description #1652
Conversation
addresses exercism#1651 - brought to y'all by the Julia Gang
SaschaMann
changed the title
|
This test case is already given as example in the description, but not covered by tests. I'd argue it's a bug fix and not covered by #1560. |
I was a `}`
bump version
|
I'd argue that it is not a bug and that this PR is blocked by #1560. As stated by @iHiD in the body of that Issue a test would have to be wrong, not merely missing, to be considered a bug:
Emphasis added. Though it would take some effort to survey them all, I believe there are a number of description.md files mention examples that do not literally exist in the tests. Certainly there are many descriptions that make assumptions / implications not actually addressed in tests. If this addition isn't actually testing anything new and otherwise uncovered in the tests -- which it isn't obviously doing, as all three colors appear in the same orientation in other tests -- then I'd argue it's not actually necessary, on top of not really being a bug fix. |
Co-Authored-By: Ryan Potts <rpottsoh@users.noreply.github.com>
|
@miguelraz thanks for the making the change I requested. I have to agree with @yawpitch though. I recommend following the suggestion outlined in #1560 that suggests that changes be added at the local (track) level. This test case could be added to the track where you came across this issue in the canonical data. @exercism/julia was it? Personally I think the suggested change is a good one for the canonical data. I am going to close this PR for now. This PR should be reconsidered whenever the restrictions of #1560 are reversed. |
|
Thanks, I think @SaschaMann can help with the Julia local changes. |
|
I've reopening with the |
|
@SaschaMann Nice idea. I believe y'all can also now convert PRs to drafts (I've just done that here) |
|
I believe #1560 is now reversed and this can be merged @SaschaMann . |
|
It needs to be rebased, I think. The version can be dropped and the test case itself needs a UUID now. |
There was a problem hiding this comment.
I'm not sure if 3300 ohms is the correct return value for this case. The description says:
When we get more than a thousand ohms, we say "kiloohms".
3300 is more than a thousand, so my interpretation of the description would be that the expected value is 3,3 kiloohms.
|
Oh, lol, should we get into locale specific decimal separators too? :)
…On Mon, 14 Mar 2022, 16:29 Angelika Tyborska, ***@***.***> wrote:
***@***.**** requested changes on this pull request.
I'm not sure if 3300 ohms is the correct return value for this case. The
description says:
When we get more than a thousand ohms, we say "kiloohms".
3300 is more than a thousand, so my interpretation of the description
would be that the expected value is 3,3 kiloohms.
—
Reply to this email directly, view it on GitHub
<#1652 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABNZA6NZ4JUWAD4DXPK3QXTU75SODANCNFSM4MTC5KEQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you are on a team that was mentioned.Message
ID: ***@***.***
com>
|
|
@cmcaine Huh? I'm not sure where that suggestion comes from. The return values are described as objects with |
Some implementations of this exercise use strings as expected value, e.g. C#'s. Doesn't change your argument because the language would deal with it, but it's worth keeping that in mind for these tests. |
|
https://neurophysics.ucsd.edu/courses/physics_120/resistorcharts.pdf switches at exactly 1000 ohms. I find that compelling enough to fix the description to |
resolves #1651