custom-set: update generator #622
Conversation
|
This generator also uses TestGroups and GroupComment, like the clock exercise generator, and also introduces a PropertyMatch function which may also be reusable. I thought it best to hold any re-factor of clock and custom-set generator with a change to gen/gen.go until later. Handle with separate issue perhaps. |
There was a problem hiding this comment.
Looks good to me @leenipper , but I'd appreciate another reviewer, and have a couple of queries.
exercises/custom-set/.meta/gen.go
Outdated
| Expected []int | ||
| func (c OneCase) PropertyMatch(property string) bool { return c.Property == property } | ||
|
|
||
| func (groups TestGroups) GroupComment(property string) string { |
There was a problem hiding this comment.
I should've done this in the Clock PR (#620), but I'm not sure I understand this GroupComment. I'm not questioning it, I'd just benefit from an explanation of what it's doing and why you've added it @leenipper. And I think it would be good to move any explanation into a doc comment as well, for future reference and future contributors.
exercises/custom-set/.meta/gen.go
Outdated
| {{with .J.Groups}} | ||
| // {{ .GroupComment "subset"}} | ||
| {{end}} var subsetCases = []binBoolCase{ {{range .J.Groups}} {{range .Cases}} | ||
| {{if .PropertyMatch "subset"}} { // {{.Description}} |
There was a problem hiding this comment.
Is it possible to pass "subset"/"disjoint"/"equal" etc. to the template so they can be nested as they currently are?
There was a problem hiding this comment.
My attempts to do exactly that have not met with success yet. I have certainly tried to do it, since there is a nice pattern there. I will study it more and see.
There was a problem hiding this comment.
I assumed you had 😄 I'll have a look too if I get time. As you say there is a nice pattern there.
|
see #629 |
exercises/custom-set/.meta/gen.go
Outdated
| // it is used in order to pass multiple parameters in a template call. | ||
| func dict(values ...interface{}) (map[string]interface{}, error) { | ||
| if len(values)%2 != 0 { | ||
| return nil, errors.New("invalid dict call") |
There was a problem hiding this comment.
is it worth having a more explicit error message? invalid dict call: odd number of values given or somesuch?
There was a problem hiding this comment.
invalid dict call: odd number of values given
That is better. Even if only for readers of the code.
| // GroupComment looks in each of the test case groups to find the | ||
| // group for which every test case has the .Property matching given property; | ||
| // it returns the .Description field for the matching property group, | ||
| // or a 'Note: ...' if no test group consistently matches given property. |
| {{template "eleOp" dict "PropertyType" "add" "Groups" .J.Groups}} | ||
| {{template "binaryOp" dict "PropertyType" "intersection" "Groups" .J.Groups}} | ||
| {{template "binaryOp" dict "PropertyType" "difference" "Groups" .J.Groups}} | ||
| {{template "binaryOp" dict "PropertyType" "union" "Groups" .J.Groups}} |
There was a problem hiding this comment.
You found a way! 🎉 You must be pleased with this! 😄
There was a problem hiding this comment.
Others discovered this solution prior; some googling helped me find it. Looks odd at first, but solves the problem well.
|
LGTM 👍 though again I'd appreciate a second reviewer, @petertseng @tleen @ferhatelmas |
|
Should this series be squashed and force pushed again, using "For issue #629" in commit message ? Update: I'll go ahead and do this. |
For issue exercism#629. Use .Header in generator template. Align template with latest canonical-data.json. Use a OneCase struct type to contain the whole set of possible json objects for the various test groups. Add istrSlice as conversion helper for .Expected. Rework each define & template call to also generate the var for the test cases. Use .GroupComment helper function to output the description for the test case group. Use dict helper function to pass multiple parameters to a template. Update test program to change hasCases => containsCases, and eqCases => equalCases; the new templates output each test case array var from within the template and simply prepend the property to "Cases" for the variable name.
leenipper
force-pushed
the
custom-set-update-generator
branch
from
a905e1c to
7aa9e52
Compare
exercises/custom-set/.meta/gen.go
Outdated
| } | ||
| Property string | ||
| Set []int // "empty"/"contains"/"add" cases | ||
| Set1 []int // "empty"/"contains"/"add" cases |
There was a problem hiding this comment.
I think the cases for Set1 should be the same as for Set2, not Set?
There was a problem hiding this comment.
Indeed, you are correct.
Set1/Set2 apply to same test case groups.
For issue #604.
Use .Header in generator template.
Align template with latest canonical-data.json.
Use a OneCase struct type to contain the whole
set of possible json objects for the various
test groups.
Add istrSlice as conversion helper for .Expected.