[WIP] say: update tests #867

MatForsberg · 2017-09-20T19:54:54Z

In response to issue #837: README suggests to test -1 but the unit tests expect uint64

Changed documentation to no longer suggest tests with negative numbers and clarify necessary error handling.

robphoenix · 2017-09-21T11:30:14Z

@MatForsberg I've changed the base branch from master to nextercism

MatForsberg · 2017-09-29T16:54:16Z

What do i need to do to get this accepted?

robphoenix · 2017-09-29T16:58:57Z

Hi Mat, I'm really sorry, these past few weeks have been a bit crazy. I've just not had time to catch up on this. I will make the effort to properly review this early next week. Again, sorry for the delay.

On Fri, 29 Sep 2017 at 17:54, Mat Forsberg ***@***.***> wrote: What do i need to do to get this accepted? — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#867 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AI1BRIbnjMr6kVfcmrDtxBF5dsp3q4bwks5snSC6gaJpZM4PeY9v> .

-- { https://bordeltabernacle.github.io/ }

cored · 2017-09-29T17:43:45Z

exercises/say/example.go

-func Say(n uint64) string {
+func Say(n uint64) (string, error) {
+	if n >= 1000000000000 {
+		return "", &errorString{"Value out of range."}


@MatForsberg I'm curious about why introduce a new error type instead of using errors.New('value out of range')? Your new type is not carrying extra data. Also I believe that the string should be in lower case entirely. You can get more information in here:
https://github.com/golang/go/wiki/Errors

Thank you. I have made the suggested changes. I did things that way because I am new to go and that is how i learned to do errors.

Good eye @cored thanks!

Thanks @cored 👍

Some scales where not being used because the instructions of the project specify the number given will be less than one trillion. Changed error handling to use errors library and mad error text all lower case.

tleen · 2017-09-29T20:54:05Z

Same here, been swamped a bit. I set aside some gotrack time this weekend and this is top on the list!

tleen · 2017-10-01T02:46:48Z

@MatForsberg this is a lot like #870 in regards to editing the README.md directly. Take a look at my comment for that PR.

Because the README is autogenerated now we actually want to make these modifications in a .meta/description.md file. You can see the information on the meta files in our README. Then we can use the configlet to generate the README automatically.

So I think what we should to do here is:

add a ./exercises/say/.meta/description.md file with the new information in the README. (Copy the description.md from say and modify to be more specific).
also copy the metadata file into ./exercises/say/.meta/metadata.yml (configlet has a bug where it wont process the description.md unless there is a metadata.yml file too: Feature: Add --probSpecsDir option configlet#65.)
install configlet locally
then in the root of the go track repo execute configlet generate --only say . which will generate the README.md file for say with the changes you made to the description.md file
then commit the new generated README.md, and the contents of the .meta/ dir.

Whew! Seems like a lot but once you get how the configlet is generating the README files it all makes sense. It is trying to balance the common information from the problem-specifications repo with trask specific stuff. You have identified that our exercise has some really specific instructions that override the general info from problem-specifications so it is a good idea to override the description from there and write our own. It is just that we need to do in in this way to make global changes to the READMEs also work.

It is well thought out I promise! 😅

robphoenix · 2017-10-03T11:07:36Z

also copy the metadata file into ./exercises/say/.meta/metadata.yml (configlet has a bug where it wont process the description.md unless there is a metadata.yml file too: exercism/configlet#65.)

There is a fix in the pipeline for this: exercism/configlet#84

robphoenix

Thanks for this @MatForsberg and sorry you've had to wait so long for an appropriate response.

I think it's best if we change the exercise to take a signed int64 rather than the uint64 it currently does. This would mean we wouldn't need to diverge from the description & canonical-data in the problem-specifications repo.

This means the README wouldn't need any changes. The test cases would need to be updated to reflect those that are in the problem-specifications repo: https://github.com/exercism/problem-specifications/blob/master/exercises/say/canonical-data.json and the example to be updated to work with the new test cases.

How does that sound to you?

robphoenix · 2017-10-05T11:23:46Z

exercises/say/example.go


-func Say(n uint64) string {
+func Say(n uint64) (string, error) {


I think this should be changed to a signed int64, so we can validate negative numbers also.

I like the change in the function signature to include an error. As we can't return -1 when n is out of range this is the best way to go. 👍

robphoenix · 2017-10-05T11:29:02Z

exercises/say/example.go

 	case n < 1000:
 		h := small[n/100] + " hundred"
 		s := n % 100
 		if s > 0 {
-			h += " " + Say(s)
+			temp, _ := Say(s)


I'm not sure about this variable name, I understand it's to signify temporary, but I think x would suffice, the use of temp rather than a letter seems to infer more importance on it than is necessary.

I also think this error should be handled. At this point in the function it shouldn't error out, but still it's better to handle it than ignore it I think.

robphoenix · 2017-10-05T11:30:00Z

exercises/say/README.md

@@ -9,17 +9,17 @@ Handle the basic case of 0 through 99.
 If the input to the program is `22`, then the output should be
 `'twenty-two'`.

-Your program should complain loudly if given a number outside the
-blessed range.
+Your program should throw an error if given a number outside the


I think this can stay as it is.

robphoenix · 2017-10-05T11:31:09Z

exercises/say/README.md

- -1
- 100
+- 99 
+- 100 (currently an error case)


With changing the function to use an unsigned int64 I don't think we need to stray from the description in the problem-specifications repo.

robphoenix · 2017-10-10T09:46:22Z

This PR has been superseded by #885

Thanks @MatForsberg and sorry this won't be merged.

Mat Forsberg added 2 commits September 20, 2017 15:59

Changed documentation and updated example and test to reflect changes

9ae0007

Changed documentation to no longer suggest tests with negative numbers and clarify necessary error handling.

changed "complain loudly" in README.md and formatted example.go

f90bc45

robphoenix changed the base branch from master to nextercism September 21, 2017 11:29

robphoenix added the Nextercism label Sep 21, 2017

cored reviewed Sep 29, 2017

View reviewed changes

MatForsberg added 2 commits September 29, 2017 14:12

Removed unused scales and fixed error type

c3d2389

Some scales where not being used because the instructions of the project specify the number given will be less than one trillion. Changed error handling to use errors library and mad error text all lower case.

Merge branch 'nextercism' into ImproveSayReadMe

7b99659

robphoenix suggested changes Oct 5, 2017

View reviewed changes

robphoenix changed the title ~~[WIP] Say README suggests to test -1 but the unit tests expect uint64 #837~~ [WIP] say: update tests Oct 5, 2017

robphoenix force-pushed the nextercism branch 2 times, most recently from 82fb9a6 to 3e89b2c Compare October 10, 2017 08:06

robphoenix closed this Oct 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] say: update tests #867

[WIP] say: update tests #867

MatForsberg commented Sep 20, 2017 •

edited by robphoenix

Loading

robphoenix commented Sep 21, 2017

MatForsberg commented Sep 29, 2017

robphoenix commented Sep 29, 2017 via email

cored Sep 29, 2017

MatForsberg Sep 29, 2017

tleen Oct 1, 2017

robphoenix Oct 5, 2017

tleen commented Sep 29, 2017

tleen commented Oct 1, 2017

robphoenix commented Oct 3, 2017

robphoenix left a comment

robphoenix Oct 5, 2017

robphoenix Oct 5, 2017

robphoenix Oct 5, 2017

robphoenix Oct 5, 2017

robphoenix commented Oct 10, 2017


		func Say(n uint64) string {
		func Say(n uint64) (string, error) {

[WIP] say: update tests #867

[WIP] say: update tests #867

Conversation

MatForsberg commented Sep 20, 2017 • edited by robphoenix Loading

robphoenix commented Sep 21, 2017

MatForsberg commented Sep 29, 2017

robphoenix commented Sep 29, 2017 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tleen commented Sep 29, 2017

tleen commented Oct 1, 2017

robphoenix commented Oct 3, 2017

robphoenix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robphoenix commented Oct 10, 2017

MatForsberg commented Sep 20, 2017 •

edited by robphoenix

Loading