say: generate tests #885

shaleh · 2017-10-05T19:37:49Z

This brings the Go track in line with others. Say now returns (string, bool) so it can indicate when a value is out of range.

Part of #605.
Closes #837

Part of exercism#605. Closes exercism#837

shaleh · 2017-10-05T19:46:15Z

I chose to use the ok idiom for the error reporting here. The code can be switched to using error if that is preferred. Ideally this is documented somewhere. Do we know where the right place for that is?

tleen · 2017-10-06T14:06:51Z

exercises/say/.gitignore

@@ -0,0 +1 @@
+.meta/gen


Why ignore .meta/gen here?

tleen · 2017-10-06T14:11:35Z

My feeling on it is that unless you have more information to convey than worked/didn't work you use err, otherwise ok.

shaleh · 2017-10-06T14:43:14Z

habit. Checking in binaries is usually frowned on.

…

On October 6, 2017 7:07:03 AM PDT, Tom Leen ***@***.***> wrote: tleen commented on this pull request. > @@ -0,0 +1 @@ +.meta/gen Why ignore .meta/gen here? -- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: #885 (review)

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

shaleh · 2017-10-06T14:44:53Z

Some over achiever might want to report too big or too small. I think ok is fine here

…

On October 6, 2017 7:11:36 AM PDT, Tom Leen ***@***.***> wrote: My feeling on it is that unless you have more information to convey than worked/didn't work you use `err`, otherwise `ok`. -- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: #885 (comment)

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

tleen · 2017-10-06T14:48:26Z

Good habit! I have it too! I only noticed because it slipped by me in anagram.

I'm ok with ok it's probably not a bad thing to bring into some exercises since it is a pattern in Go. @robphoenix @leenipper thoughts?

tleen

Def remove the gitignore file so we can track gen.

shaleh · 2017-10-06T15:10:08Z

Will do. Probably not today.

…

On October 6, 2017 7:49:07 AM PDT, Tom Leen ***@***.***> wrote: tleen requested changes on this pull request. Def remove the gitignore file so we can track gen. -- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: #885 (review)

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

ferhatelmas · 2017-10-06T16:19:37Z

Def remove the gitignore file so we can track gen.

@tleen I don't understand your above comment because even with gitignore, gen.go is tracked and we don't want to track artifact gen. I think ignore is good since it would prevent errors but place isn't the best. If we want to provide, it should be in the repo level for all exercises.

tleen · 2017-10-06T16:36:11Z

Sorry @ferhatelmas I wasn't clear. We wouldn't be adding gen anyway, as you can run the gen without building it. I'm actually ok with it getting added to a commit so we can catch that error. The local .gitignore is a nice safety, but as you said should be done as a global policy, not at the exercise level.

ferhatelmas · 2017-10-06T16:38:51Z

@tleen Thanks for clarification. We're on the same page then.

ferhatelmas

It's good to go when gitignore is removed.

leenipper

Suggest one small change in TestSay.

leenipper · 2017-10-07T22:27:25Z

exercises/say/say_test.go

+				t.Fatalf("FAIL: %s\nExpected error but received: %v", tc.description, actual)
+			}
+		} else if !ok {
+			t.Fatalf("FAIL: %s\nDid not expect an error", tc.description)


Suggest adding:

+ } else if actual != tc.expected { + t.Fatalf("FAIL: %s\nExpected: %v\nActual: %v", tc.description, tc.expected, actual)

leenipper · 2017-10-07T22:29:02Z

exercises/say/example.go

@@ -8,7 +8,14 @@ var tens = []string{"ones", "ten", "twenty", "thirty", "forty",
 var scale = []string{"thousand", "million", "billion",
 	"trillion", "quadrillion", "quintillion"}

-func Say(n uint64) string {
+func Say(n int64) (value string, ok bool) {


I'm good with ok bool here. The standard library has instances like it.

Does anybody know if/where there is some sort of community accepted definition or guideline as for when to use an err vs an ok?

I have not found a straight out "accepted definition". There is discussion. Here is one from 2015: https://groups.google.com/forum/#!topic/golang-nuts/e5qIlnhpgg0
The two points I note are:

when only one possible error condition, a bool is fine.

using an error is more flexible for future updates.

I think for Say, it meets no. 1 (out of range is the !ok case), and doesn't warrant no. 2.

Thanks @leenipper that was sort of my take on it too. Good to see I was not alone in it!

@leenipper thanks for catching that. New patch on the way soon.

The preference is for a top level one.

Part of #605. Closes #837

Part of exercism#605. Closes exercism#837

Say: generate tests

a4ba93b

Part of exercism#605. Closes exercism#837

tleen reviewed Oct 6, 2017

View reviewed changes

exercises/say/.gitignore Outdated

@@ -0,0 +1 @@

.meta/gen

Copy link

Member

tleen Oct 6, 2017

Choose a reason for hiding this comment

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

Why ignore .meta/gen here?

tleen suggested changes Oct 6, 2017

View reviewed changes

ferhatelmas changed the title ~~Say: generate tests~~ say: generate tests Oct 6, 2017

ferhatelmas suggested changes Oct 6, 2017

View reviewed changes

leenipper suggested changes Oct 7, 2017

View reviewed changes

Sean 'Shaleh' Perry added 2 commits October 9, 2017 08:35

Add back in actual v. expected test

3fdb6c8

Remove local .gitignore.

8808ea8

The preference is for a top level one.

tleen approved these changes Oct 9, 2017

View reviewed changes

leenipper approved these changes Oct 10, 2017

View reviewed changes

tleen merged commit 3e89b2c into exercism:nextercism Oct 10, 2017

robphoenix pushed a commit that referenced this pull request Oct 10, 2017

say: generate tests (#885)

82fb9a6

Part of #605. Closes #837

robphoenix pushed a commit to robphoenix/exercism-go that referenced this pull request Oct 10, 2017

say: generate tests (exercism#885)

7013cf0

Part of exercism#605. Closes exercism#837

robphoenix pushed a commit to robphoenix/exercism-go that referenced this pull request Oct 10, 2017

say: generate tests (exercism#885)

7907379

Part of exercism#605. Closes exercism#837

robphoenix mentioned this pull request Oct 10, 2017

[WIP] say: update tests #867

Closed

petertseng mentioned this pull request Oct 31, 2017

Write generators for all exercises #605

Open

48 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

say: generate tests #885

say: generate tests #885

shaleh commented Oct 5, 2017

shaleh commented Oct 5, 2017

tleen Oct 6, 2017

tleen commented Oct 6, 2017

shaleh commented Oct 6, 2017 via email

shaleh commented Oct 6, 2017 via email

tleen commented Oct 6, 2017

tleen left a comment

shaleh commented Oct 6, 2017 via email

ferhatelmas commented Oct 6, 2017

tleen commented Oct 6, 2017

ferhatelmas commented Oct 6, 2017

ferhatelmas left a comment

leenipper left a comment

leenipper Oct 7, 2017

leenipper Oct 7, 2017

tleen Oct 7, 2017

leenipper Oct 8, 2017

tleen Oct 8, 2017

shaleh Oct 9, 2017

say: generate tests #885

say: generate tests #885

Conversation

shaleh commented Oct 5, 2017

shaleh commented Oct 5, 2017

tleen Oct 6, 2017

Choose a reason for hiding this comment

tleen commented Oct 6, 2017

shaleh commented Oct 6, 2017 via email

shaleh commented Oct 6, 2017 via email

tleen commented Oct 6, 2017

tleen left a comment

Choose a reason for hiding this comment

shaleh commented Oct 6, 2017 via email

ferhatelmas commented Oct 6, 2017

tleen commented Oct 6, 2017

ferhatelmas commented Oct 6, 2017

ferhatelmas left a comment

Choose a reason for hiding this comment

leenipper left a comment

Choose a reason for hiding this comment

leenipper Oct 7, 2017

Choose a reason for hiding this comment

leenipper Oct 7, 2017

Choose a reason for hiding this comment

tleen Oct 7, 2017

Choose a reason for hiding this comment

leenipper Oct 8, 2017

Choose a reason for hiding this comment

tleen Oct 8, 2017

Choose a reason for hiding this comment

shaleh Oct 9, 2017

Choose a reason for hiding this comment