Skip to content
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

Merged
merged 16 commits into from
Sep 20, 2017
Merged

collatz-conjecture #593

merged 16 commits into from
Sep 20, 2017

Conversation

Average-user
Copy link
Member

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.

@petertseng
Copy link
Member

config.json, uuid part of the excercise.

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-)
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

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
Copy link
Member

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

Copy link
Member

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
Copy link
Member

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

@Average-user
Copy link
Member Author

Well. I'm glad to have guessed. But i still without understand Travis, Will ever let me free?

@Average-user
Copy link
Member Author

Travis gives me an error of another excercise

Registering accumulate-0.0.0...
accumulate-0.0.0: test (suite: test)
empty accumulation
accumulate squares
accumulate upcases
accumulate reversed strings
accumulate recursively
accumulate non-strict FAILED [1]
Failures:
  test/Tests.hs:35: 
  1) accumulate non-strict
       uncaught exception: ErrorCall (accumulate should be even lazier, don't use reverse!
       CallStack (from HasCallStack):
         error, called at test/Tests.hs:36:45 in main:Main)
Randomized with seed 1712205088
Finished in 0.0005 seconds
6 examples, 1 failure
Completed 2 action(s).
Test suite failure for package accumulate-0.0.0
    test:  exited with: ExitFailure 1
Logs printed to console```

@petertseng
Copy link
Member

Travis gives me an error of another excercise

testing /home/travis/build/exercism/haskell/exercises/accumulate/examples/fail-reverse - should build, but fail tests

It should fail, and it did, so no problem. Please look for problems related to collatz-conjecture

@Average-user
Copy link
Member Author

But why still a red X on my pull request?

And i was thinking that Knuth shuffle algorithm, might be a good excercise

@petertseng
Copy link
Member

But why still a red X on my pull request?

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 stack test in the collatz-conjecture directory on your computer? Alternatively, what do you get if you try to run https://github.com/exercism/haskell/blob/master/bin/test-all-examples ? Something like bin/test-all-examples exercises/collatz-conjecture ?

And i was thinking that Knuth shuffle algorithm, might be a good excercise

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.

@Average-user
Copy link
Member Author

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 ...)

@Average-user
Copy link
Member Author

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.
But it's a different exercise, not the Knuth algorithm.

, expected = 0
}
, Case { description = "collatz 2 require 1 step."
, number = 2
Copy link
Member

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:

  1. Use the test cases that appear in https://github.com/exercism/problem-specifications/blob/master/exercises/collatz-conjecture/canonical-data.json
  2. 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

Copy link
Member Author

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 ?

Copy link
Member

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:

  1. Use 1.1.1.1 and the cases in https://github.com/exercism/problem-specifications/blob/master/exercises/collatz-conjecture/canonical-data.json
  2. 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.

Copy link
Member

@petertseng petertseng left a 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
Copy link
Member

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": [
Copy link
Member

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)

Copy link
Member Author

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
Copy link
Member

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"
Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

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)

Copy link
Member

@petertseng petertseng left a 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.

@Average-user
Copy link
Member Author

No, do it. I'm ok with that

@petertseng petertseng merged commit 7dc8cf2 into exercism:master Sep 20, 2017
@petertseng
Copy link
Member

Thank you for implementing this exercise!

@@ -0,0 +1,52 @@
{-# OPTIONS_GHC -fno-warn-type-defaults #-}
Copy link
Member

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.

Copy link
Member

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.

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