-
-
Notifications
You must be signed in to change notification settings - Fork 657
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
grains: add generator #959
Conversation
Add .meta/gen.go to generate cases_test.go. Make use of zero-value of expectError bool. Update test program to use generated test case array. The test case value for the 'Total()' is retained in TestTotal() since there wasn't an array of cases in the canonical-data.json for the 'total' property, and there is only one valid return value. For exercism#605.
exercises/grains/.meta/gen.go
Outdated
if ans == float64(-1) { | ||
return false, 0 | ||
} | ||
return ok, uint64(ans) |
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.
For my own curiousity, why not return true
instead of ok
here? The ok val is a reuse of the ok val for a different expression which is (very slightly) confusing and it has to be true to get here anyway.
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.
Yes, agree.
Description string | ||
Cases []json.RawMessage `property:"RAW"` | ||
SquareCases []SquareCase `property:"square"` | ||
// Note: canonical-data.json has a element of "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.
Have you or @ferhatelmas encountered an other canonical-data files with a cases.cases structure like this one? It seems odd. Wonder if there may be a better way to represent the data in that file we could suggest upstream.
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 are several which have nested cases.cases but only one property value; these are not a problem. Others also have differing property values, which make it a little harder to handle. Two exercises, custom-set and variable-length-quantity have generators that use a PropertyMatch technique. Four others use the RAW special handling with classifyByProperty (allergies, run-length-encoding, clock, and grains). A different structuring in canonical data could help to generate separate test case arrays for different property values. A change would require a retrofit for current generators however.
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.
When I first saw, I was shocked but then checked json schema in problem-specification and it was valid. Thus, even if it's harder to handle, thought that was the decision and tried to live with.
exercises/grains/.meta/gen.go
Outdated
Expected interface{} | ||
} | ||
|
||
func (c SquareCase) Valid() bool { |
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.
Perhaps HasAnswer
is a more descriptive term for this? It took me a second to realize it was the error check for Answer 😄. Not having an answer is a valid test case, just that it is looking for an error rather than an answer value? It makes that if
check in the template below straightforward.
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 agree, HasAnswer is more descriptive.
} | ||
|
||
func (c SquareCase) EditedDescription() string { | ||
// Go doesn't raise exceptions, so replace the wording in .Description. |
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.
Do you think this is worth bringing up in problem-specifications? Exception seems to be the narrower term here, while Error is the more general. If anything exception based languages should be rewriting these descriptions if necessary. I've always considered Exceptions to be a sub-term for Errors in generic conversation, but not the other way around.
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.
Yes. I was actually surprised to see the exception wording in the canonical data file. I thought it was very tilted toward languages that use exceptions. Error is a better term to use there.
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.
Agreed here.
In generator, rename Valid to HasAnswer. Use a return true instead of return ok which was unclear. Update function description for determineExpected(). Remove extraneous unused func GroupShortName.
Thanks @tleen. |
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.
LGTM
Description string | ||
Cases []json.RawMessage `property:"RAW"` | ||
SquareCases []SquareCase `property:"square"` | ||
// Note: canonical-data.json has a element of "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.
When I first saw, I was shocked but then checked json schema in problem-specification and it was valid. Thus, even if it's harder to handle, thought that was the decision and tried to live with.
} | ||
|
||
func (c SquareCase) EditedDescription() string { | ||
// Go doesn't raise exceptions, so replace the wording in .Description. |
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.
Agreed here.
Add .meta/gen.go to generate cases_test.go.
Make use of zero-value of expectError bool.
Update test program to use generated test case array.
The test case value for the 'Total()' is retained in TestTotal()
since there wasn't an array of cases in the canonical-data.json
for the 'total' property, and there is only one valid return value.
For #605.