Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

hamming: Simplify two tests so the difference is easily spotted. #953

Merged
merged 1 commit into from
Oct 15, 2017

Conversation

Stargator
Copy link
Contributor

I feel like the cases "non-unique character in first strand" and "non-unique character in second strand" are not clear in how the respective strand has a non-unique character.

So I suggest simplifying it. Otherwise, I would be fine with switching them as seen below. Where it's clear by the pattern of the other strand that the strand with the non-unique character doesn't follow the expected pattern.

{
      "description": "non-unique character in first strand",
      "property": "distance",
      "strand1": "AGG",
      "strand2": "AGA",
      "expected": 1
    },
    {
      "description": "non-unique character in second strand",
      "property": "distance",
      "strand1": "AGA",
      "strand2": "AGG",
      "expected": 1
    },```

Copy link
Contributor

@Insti Insti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this 👍
I also think the minor version increment is correct here.

@Stargator Stargator self-assigned this Oct 15, 2017
@Stargator Stargator merged commit 5bf4353 into exercism:master Oct 15, 2017
@Stargator Stargator deleted the revise-hamming-cases branch October 15, 2017 15:48
@petertseng
Copy link
Member

I agree that minor version increment is correct because test input was changed, and it is a case listed in https://github.com/exercism/problem-specifications/blob/master/README.md#minor-version-changes.

I would also like to point out that this PR did not increase minor version, but increased patch version.

This doesn't matter for any track I maintain, so I don't need anything to change. If it matters for your track, you should change the minor version.

@Insti
Copy link
Contributor

Insti commented Oct 15, 2017

this PR did not increase minor version, but increased patch version.

I think that the change that happened was the correct one.

So it seems I disagree with the documentation.
See also #938

@petertseng petertseng changed the title Simplify two tests so the difference is easily spotted. hamming: Simplify two tests so the difference is easily spotted. Nov 4, 2017
petertseng added a commit to petertseng/exercism-ceylon that referenced this pull request Nov 5, 2017
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants