[ISBN Verifier] Error in tests #2601
Conversation
…y the verification digit PR according to [this](https://forum.exercism.org/t/isbn-verifier-error-in-tests-that-check-if-the-x-character-is-only-the-verification-digit/19639) topic.
|
Hello. Thanks for opening a PR on Exercism 🙂 We ask that all changes to Exercism are discussed on our Community Forum before being opened on GitHub. To enforce this, we automatically close all PRs that are submitted. That doesn't mean your PR is rejected but that we want the initial discussion about it to happen on our forum where a wide range of key contributors across the Exercism ecosystem can weigh in. You can use this link to copy this into a new topic on the forum. If we decide the PR is appropriate, we'll reopen it and continue with it, so please don't delete your local branch. If you're interested in learning more about this auto-responder, please read this blog post. Note: If this PR has been pre-approved, please link back to this PR on the forum thread and a maintainer or staff member will reopen it. |
Keftcha
force-pushed
the
ISBN-Verifier
branch
2 times, most recently
from
2c0a5e3 to
b53040e
Compare
Thanks to @Cool-Katt for linking the documentation I needed.
Keftcha
force-pushed
the
ISBN-Verifier
branch
from
b53040e to
5610496
Compare
| { | ||
| "uuid": "0153f517-3008-4832-a168-8155c2ec0d94", | ||
| "reimplements": "28025280-2c39-4092-9719-f3234b89c627", | ||
| "description": "X is only valid as a check digit - X character is substituted for 10 and multiplied", |
There was a problem hiding this comment.
Do we need to keep this test at all of its superseded by the other one that also catches the edge cases?
If yes, then I'd vote to change the names to something more distinct for the new test, so we can avoid the reimplementation. (I don't have a good name suggestion at the moment, sry)
There was a problem hiding this comment.
If we're not adding the other tests, this description doesn't need to change.
There was a problem hiding this comment.
Do we need to keep this test at all of its superseded by the other one that also catches the edge cases?
I've commented in the forum topic, but one does not supersede the other. The current one checks when X is substituted for 10 and multiplied by the position (as the other digits are). The proposed one checks when X is substituted for 10 without being multiplied by the position, but fails to assert when it is multiplied by the position. So, they are complementary tests.
| }, | ||
| { | ||
| "uuid": "fdb14c99-4cf8-43c5-b06d-eb1638eff343", | ||
| "description": "X is only valid as a check digit - X character is substituted for 10 and not multiplied", |
There was a problem hiding this comment.
I'm not convinced this test is needed.
There was a problem hiding this comment.
I'm not convinced this test is needed.
This is the test proposed in the topic. The other one is the current test already present in the specs, only renamed.
| }, | ||
| { | ||
| "uuid": "47fa54cb-507e-45d3-901a-096be447dfd9", | ||
| "description": "X is only valid as a check digit - X character is ignored altogether", |
There was a problem hiding this comment.
I'm not convinced this test is needed.
| { | ||
| "uuid": "0153f517-3008-4832-a168-8155c2ec0d94", | ||
| "reimplements": "28025280-2c39-4092-9719-f3234b89c627", | ||
| "description": "X is only valid as a check digit - X character is substituted for 10 and multiplied", |
There was a problem hiding this comment.
If we're not adding the other tests, this description doesn't need to change.
| "reimplements": "28025280-2c39-4092-9719-f3234b89c627", | ||
| "description": "X is only valid as a check digit - X character is substituted for 10 and multiplied", | ||
| "comments": [ | ||
| "Rename this test due to multiple tests on X that should be only valid as a check digit" |
There was a problem hiding this comment.
This comment looks like a commit comment. Looking at the test data and not the changes, this comment isn't useful so it should not be here.
There was a problem hiding this comment.
Is something like «Add a more precise description» better ?
There was a problem hiding this comment.
For a changelog? Sure. For the canonical data? There's no need for the canonical data file to contain change history. That's what the git log is for. The comments in the file should be useful to explain the data in the file. The comments in the file should not be used to explain the history of the file.
4 participants
Check if the
Xcharacter is only the verification digitPR according to this topic.
Resolve exercism/python#4014