Skip to content

Hamming: Add a test case to avoid wrong recursion solution #796

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

Merged
merged 3 commits into from
Feb 14, 2019
Merged

Hamming: Add a test case to avoid wrong recursion solution #796

merged 3 commits into from
Feb 14, 2019

Conversation

tqa236
Copy link
Contributor

@tqa236 tqa236 commented Jan 30, 2019

This solution will pass all the current test cases but fail on the new one:

module Hamming (distance) where


distance :: String -> String -> Maybe Int
distance [] []         = Just 0
distance (x:xs) (y:ys)
  | length(xs) /= length(ys) = Nothing
  | x /= y = fmap (1 + ) (distance xs ys)
  | x == y = distance xs ys

@sshine sshine changed the title Add a test case to avoid wrong recursion solution. Hamming: Add a test case to avoid wrong recursion solution Feb 1, 2019
@sshine
Copy link
Contributor

sshine commented Feb 1, 2019

Thanks a lot for your pull request!

And you are quite right, this test case is missing.

If you don't mind, I would like to propagate this test case to the central Exercism repository for test cases shared across language tracks, since I imagine this case may trigger at least other functional languages.

Before this can happen, I just noticed that the Hamming exercise is one version bump behind on the Haskell track. I've bumped the Hamming test suite version to 2.2.0 in #797, so that you don't also have to fix this.

  1. Fork the repo exercism/problem-specifications
  2. In that repo, make an update to the file exercises/hamming/canonical-data.json: Add this test case and increase "version": "2.2.0" to "version": "2.3.0". Send a PR there and reference this PR by writing exercism/haskell#796 in the message.
  3. In this repo, rebase your current feature branch on upstream now that I've merged Hamming: Bump test suite to 2.2.0 #797. (Let me know if you want hints on how to do that.)
  4. In this repo, bump the version number in the file exercises/hamming/package.yaml from version: 2.2.0.9 to version: 2.3.0.10. The first three numbers are derived from the version in canonical-data, and the fourth is our track's incremental pointer.

I'll merge this PR once the canonical data is merged and the version in package.yaml reflects 2.3.0.10. :-)

This solution will pass all the current test but fail on the new one:

```
module Hamming (distance) where


distance :: String -> String -> Maybe Int
distance [] []         = Just 0
distance (x:xs) (y:ys)
  | length(xs) /= length(ys) = Nothing
  | x /= y = fmap (1 + ) (distance xs ys)
  | x == y = distance xs ys
```
Update test version
@tqa236
Copy link
Contributor Author

tqa236 commented Feb 1, 2019

Hello,

I did all 4 points you said and request a new pull request. Please tell me if you notice anything wrong.

rpottsoh pushed a commit to exercism/problem-specifications that referenced this pull request Feb 7, 2019
Add an extra test case about one empty strand based on the discussion in exercism/haskell#796 to avoid wrong recursion solution.
@tqa236
Copy link
Contributor Author

tqa236 commented Feb 7, 2019

@sshine Is this PR ok to merge now or do we still need some changes?

@sshine sshine merged commit 473b7b3 into exercism:master Feb 14, 2019
@sshine
Copy link
Contributor

sshine commented Feb 14, 2019

@tqa236: We're good. Thanks a lot, and sorry for the late response.

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.

2 participants