-
-
Notifications
You must be signed in to change notification settings - Fork 192
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
collatz-conjecture #593
collatz-conjecture #593
Conversation
You guessed correctly (I think) that you generate any arbitrary UUID and use it. I notice that https://github.com/exercism/docs/blob/master/you-can-help/implement-an-exercise-from-specification.md#configuring-the-exercise did not tell you you are supposed to generate a UUID. That is a deficiency in the documentation, which should be reported. Please notify if anything else was unclear! |
|
||
## Source | ||
|
||
All of Computer Science [http://www.wolframalpha.com/input/?i=binary&a=*C.binary-_*MathWorld-](http://www.wolframalpha.com/input/?i=binary&a=*C.binary-_*MathWorld-) |
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.
this is not the source listed in https://github.com/exercism/problem-specifications/blob/master/exercises/collatz-conjecture/metadata.yml
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.
ups, sorry. Im changing it
@@ -0,0 +1,21 @@ | |||
|
|||
name: collatz-conjecture | |||
version: 1.0.0.3 |
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.
as https://github.com/exercism/problem-specifications/blob/master/exercises/collatz-conjecture/canonical-data.json has version 1.1.1, the version that would comply with our versioning policy in #522 is 1.1.1.1
, and ensuring that the tests in the Haskell test file match those in the JSON file
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.
(or, alternatively, if you believe those tests are unsuitable for the Haskell track for any reason, let's discuss why they are unsuitable, and then use version0.1.0.1
)
@@ -0,0 +1,20 @@ | |||
name: collatz-conjecture | |||
version: 1.1.1 |
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.
needs an additional .1 serial number, resulting in 1.1.1.1
Well. I'm glad to have guessed. But i still without understand Travis, Will ever let me free? |
Travis gives me an error of another excercise
|
It should fail, and it did, so no problem. Please look for problems related to collatz-conjecture |
But why still a red X on my pull request? And i was thinking that Knuth shuffle algorithm, might be a good excercise |
I don't know, what does the Travis CI output at https://travis-ci.org/exercism/haskell/builds/277117302 say? Alternatively, what do you get if you try to
Okay, it can be suggested in https://github.com/exercism/problem-specifications . My advice is that based on exercism/problem-specifications#761 exercises that test a specific algorithm are difficult to write tests for because there is no good way to verify that a student's implementation uses that specific algorithm, rather than any other algorithm that achieves the same effect. |
Yeah you are right about the Knuth shuffle algorithm, and it's even worts if you take on count that is a SHUFFLE algorithm, not always returns the same result for the same input (well, strictly yes but ...) |
At the end the problem was that i misspelled a file name. And i followed your advice and wrote an issue on: exercism/problem-specifications#906. |
, expected = 0 | ||
} | ||
, Case { description = "collatz 2 require 1 step." | ||
, number = 2 |
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.
on my advice, you did change the problem version to 1.1.1.1, to match the 1.1.1 of https://github.com/exercism/problem-specifications/blob/master/exercises/collatz-conjecture/canonical-data.json, but you did not then use the test cases in https://github.com/exercism/problem-specifications/blob/master/exercises/collatz-conjecture/canonical-data.json. Can you do one of the two:
- Use the test cases that appear in https://github.com/exercism/problem-specifications/blob/master/exercises/collatz-conjecture/canonical-data.json
- Confirm that the test cases in https://github.com/exercism/problem-specifications/blob/master/exercises/collatz-conjecture/canonical-data.json are unsuitable for the Haskell track, and thus we cannot use that file, and thus we must change our version to 0.1.0.1
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.
so it's ok if i change it from 1.1.1.1 to 1.1.1 and use this cases ?
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.
1.1.1 is not a valid Haskell track version. The two choices are:
- Use 1.1.1.1 and the cases in https://github.com/exercism/problem-specifications/blob/master/exercises/collatz-conjecture/canonical-data.json
- Use 0.1.0.1 and whatever cases desired, with an explanation of why https://github.com/exercism/problem-specifications/blob/master/exercises/collatz-conjecture/canonical-data.json cannot be used.
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 think everything is looking good. Please expect a next review in 12 hours, but until then I have small items you could do
, Case { description = "even and odd steps" | ||
, property = "steps" | ||
, number = 12 | ||
, expected =Just 9 |
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.
add a space after the = here?
"core": false, | ||
"unlocked_by": null, | ||
"difficulty": 1, | ||
"topics": [ |
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.
since this exercise now uses Maybe, please add it to the topics list (see "rna-transcription" for example of how it is done)
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.
ok
| otherwise = collatzHelper (t+1) (x*3 + 1) | ||
|
||
collatz :: Integer -> Maybe Integer | ||
collatz x = if x <= 0 then Nothing else collatzHelper 0 x |
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.
there are two spaces after the if
, you could remove one of them, right?
|
||
cases :: [Case] | ||
cases = [ Case { description = "zero steps for one" | ||
, property = "steps" |
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.
the property
is not being used. It should be removed.
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 will, but maybe it has to be removed from https://github.com/exercism/problem-specifications/blob/master/exercises/collatz-conjecture/canonical-data.json
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.
It needs to be in problem-specifications since it tells what is being tested.
It must not be here since we already know what we are testing (it's specified in code)
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 think this is good. Please expect me to merge it in 4 hours, unless you see anything you want to change.
No, do it. I'm ok with that |
Thank you for implementing this exercise! |
@@ -0,0 +1,52 @@ | |||
{-# OPTIONS_GHC -fno-warn-type-defaults #-} |
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 don't think this line is necessary. Can you check? If you confirm it is not necessary, please send a PR that deletes it.
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 retract. I think #144 tells me it is desirable to have for exercises with numbers.
I'm implementing the collatz conjecture of the Unimplmented excercises of Haskell (excercism.io). But i wasn't sure of what to put on the config.json, uuid part of the excercise.