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

say: Rewrite tests to use hspec with fail-fast. #292

Merged
merged 1 commit into from
Sep 15, 2016
Merged

say: Rewrite tests to use hspec with fail-fast. #292

merged 1 commit into from
Sep 15, 2016

Conversation

rbasso
Copy link
Contributor

@rbasso rbasso commented Sep 12, 2016

Related to #211.

@petertseng
Copy link
Member

Did not think we would have bounds that low! But it is not for me to question

👍

@rbasso
Copy link
Contributor Author

rbasso commented Sep 13, 2016

Did not think we would have bounds that low! But it is not for me to question

You can question anything! 😄

Considering that there is no json for this exercise, we can change it as we like.

So, what would you like to change ❓

@petertseng
Copy link
Member

I suppose what I really meant was: this PR is just the conversion from HUnit to HSpec, talking about changes to the tests can come separately! But here is an OK place I suppose, since the lines are getting changed anyway.

Two separate questions for us to answer:

  • The upper limit doesn't have to be billions! Shall we also add trillion, quadrillions?
  • The lower limit doesn't have to be zero! Shall we add negative numbers? -1 could be "negative one", etc.

@rbasso
Copy link
Contributor Author

rbasso commented Sep 13, 2016

You are right, there is no reason to restrict the values.

I completely agree about dropping the upper limit, but I'm still unsure about the lower limit. Do you think that would add something interesting to the exercise?

If we decide to drop the lower limit too, there is no reason to return a Maybe anymore. How do you fell about it? Should we drop it too and remove the Maybe?

@petertseng
Copy link
Member

Take my opinion with a grain of salt since I haven't done this exercise myself in any language. I do think negatives would be somewhat interesting (probably more interesting than going to trillions, quadrillions, etc.), but the question of whether it's interesting enough to be worth adding? Well, if unsure I'll suggest to try it! But I really don't feel strongly.

It may be the case that Maybe has to be kept though because no matter how far someone goes with trillions, quadrillions, etc. there still has to be a point at which they had to stop. Unless we expect solutions to be able to arbitrarily combine latin prefixes?!

@rbasso
Copy link
Contributor Author

rbasso commented Sep 14, 2016

To patch any working solution to say negative numbers it is trivial. We just have to add this line:

inEnglish x | x < 0 = ("minus " ++) <$> inEnglish (-x)

So I don't think this would make the problem more interesting. That said, it would not be worse.

Just tell me the test cases you want to add and I'll fix this PR. 👍

@petertseng
Copy link
Member

OK. On your recommendation that it would not be more interesting, then I suggest no more tests and merge as-is. If a preponderance of other track maintaners do decide to expand when someone creates say/canonical-data.json then we can consider.

- Rewrite tests to use `hspec`.
- Remote upper-limit test.
@rbasso
Copy link
Contributor Author

rbasso commented Sep 15, 2016

I removed the upper-limit test, but left the test for -1 returning Nothing for now, until we discuss the negative inputs in x-common.

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.

Okay, so the idea is that this makes life better for the student - the student can now go to trillions if desired (the tests don't stop you now!) whereas before they could not.

OK

👍

@rbasso
Copy link
Contributor Author

rbasso commented Sep 15, 2016

If you prefer I can drop the lower limit test too. I just would like to avoid, for now, to expect valid negative outputs.

@petertseng
Copy link
Member

It's good as-is. Besides, if removed the lower limit test, then a student might ask "Why are all tests Just!"

@rbasso rbasso merged commit 66c8f4c into exercism:master Sep 15, 2016
@rbasso rbasso deleted the hspec-say branch September 15, 2016 10:19
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