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

clock: restore type-safety for different types of cases #677

Merged
merged 3 commits into from
Jun 27, 2017
Merged

clock: restore type-safety for different types of cases #677

merged 3 commits into from
Jun 27, 2017

Conversation

petertseng
Copy link
Member

@petertseng petertseng commented May 21, 2017

This PR should not be merged yet... but it should still be reviewed, to see how close to being mergeable it is.

This PR is probably ready to merge, but I'll clean up some commits and move the history to a different branch.

The question to ask is: "Would I rather write a generator using this interface than the existing one?"


Seeing how this looks. I'm not convinced it's better, but maybe it can be used as a starting point to make things better. That is why I say do not merge, but if we get a good idea out of it it could be.

Clock updated to 1.0.1 and I have code that works for it too, but I want to compare clock 1.0.0 code vs clock 1.0.0 code, so I reverted the 1.0.1 changes (they can be applied later)

}
}

if err := gen.Gen("clock", &j, t, func() {
Copy link
Member Author

Choose a reason for hiding this comment

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

could have used must here.

}

// Test adding and subtracting minutes.
var addTests = []struct {
var testTests = []struct {
Copy link
Member Author

Choose a reason for hiding this comment

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

no

gen/gen.go Outdated
@@ -90,7 +90,7 @@ func outputSource(status string, fileName string, src []byte) error {
}

// Gen generates the exercise cases_test.go file from the relevant canonical-data.json
func Gen(exercise string, j interface{}, t *template.Template) error {
func Gen(exercise string, j interface{}, t *template.Template, tmp func()) error {
Copy link
Member Author

Choose a reason for hiding this comment

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

ah right, all other calls to Gen are going to need to change here, unless the tmp becomes unnecessary. And tmp is going to need a better name (postProcess or something like that)

@petertseng
Copy link
Member Author

sad to see that this increased the number of lines, a bit unfortunate.

@petertseng
Copy link
Member Author

Trying out reflection: Does that make it better?

@petertseng
Copy link
Member Author

OK, now the clock-specific generator is about the same length (slightly shorter), which means this could be acceptable...

gen/property.go Outdated
if err := json.Unmarshal(raw, &prop); err != nil {
return err
}
tgt := properties[prop.Property]
Copy link
Member Author

Choose a reason for hiding this comment

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

TODO (only bother if the idea of Classify is deemed acceptable): these names (tgt, z, zi, zv) all are terrible, make better ones

@petertseng
Copy link
Member Author

lol more reflection

gen/property.go Outdated

const tagName = "property"

func Classify(group interface{}) error {
Copy link
Member Author

Choose a reason for hiding this comment

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

Classify doesn't need to be exported anymore.

gen/property.go Outdated
}
}
case reflect.Struct:
classify(field.Addr().Interface())
Copy link
Member Author

Choose a reason for hiding this comment

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

this line of code not tested in the clock generator... but thought it might necessary for other generators. it's if we have cases nested in other cases

gen/gen.go Outdated
@@ -130,6 +130,8 @@ func Gen(exercise string, j interface{}, t *template.Template) error {
return fmt.Errorf(`didn't contain version: %v`, err)
}

classify(j)
Copy link
Member Author

Choose a reason for hiding this comment

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

don't ignore error here.

gen/property.go Outdated
elem := field.Type().Elem()
if elem.Kind() == reflect.Struct {
for j := 0; j < field.Len(); j++ {
classify(field.Index(j).Addr().Interface())
Copy link
Member Author

Choose a reason for hiding this comment

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

don't ignore error.

gen/property.go Outdated
}
}
case reflect.Struct:
classify(field.Addr().Interface())
Copy link
Member Author

Choose a reason for hiding this comment

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

don't ignore error.

@tleen
Copy link
Member

tleen commented Jun 3, 2017

Are you looking at this as a way to do the other generations more generically? The existing generation structure is fairly easy to follow, this new one gets thick quick.

I've been thinking on the generators, this morning's comment on #605 has me wondering if we could restructure the local gen to be the struct for the json and the output template only. The tops of the files seem to be boilerplate.

Just thinking aloud here...

@petertseng
Copy link
Member Author

The primary goal: To see if this makes generating tests from JSON files with multiple values of property easier and/or makes the resulting generators more understandable.

I like that the three possible structs resulting from values of property are explicit; I prefer it much more over unioning all of them.

I don't like the added {{ if .AddCases }}. Kind of makes me wonder whether the generator should flatten everything into one level, but how will the group-level description be preserved?

Secondary goal: Determine whether added complexity caused by the reflection is understandable.

@leenipper
Copy link
Contributor

To help get a feel for this generator approach, I made the changes to the custom-set generator
based on the code of this PR, and it is here: https://github.com/leenipper/xgo/tree/clock-maybe-plus-custom-set

The custom-set generator is very likely the most complex one at present, and that is why I chose it for comparison.

I think the modified generator is much simpler, and it would have been easier to develop with this new approach. The length of the generator is a bit longer, but much of that is because the current one uses the define template mechanism which helps it be shorter. In any case, I like the simplicity and it would have saved me time and headache in the update pass done with #622, although I learned more about templates during that update.

So I think the approach has value, for the multi-property canonical-data.json instances.

Following an existing example which uses the approach makes it fairly palatable. Doing it without an example would require studying the gen/gen.go code, and of course there is a different flavor of complexity there with reflection being employed.

easier and/or makes the resulting generators more understandable.

Yes, easier and I think more understandable.

whether added complexity caused by the reflection is understandable.

It is understandable with study; but I think understanding it thoroughly may not be necessary if one follows a previous example when developing a new generator.

@petertseng
Copy link
Member Author

Interesting. It might be possible to reduce some duplication by defining a type for the binaryBool and binaryOp cases and using it (like how the tests do), though I haven't tried that.

I did mention I dislike the {{ if .AddCases }} however removing them might require more drastic changes than I like to make right now, and I'm not sure it will actually make the generators better.

This "property" tag only needs to be used in tests with more than one property, of course; we wouldn't use it for leap or the like.

It will be advantageous to ensure that existing generators without the "property" tag still work; I haven't done that yet though I expect they will work. Beyond that, if there isn't anything else we can think of to change perhaps I'll clean up the commits.

@petertseng
Copy link
Member Author

petertseng commented Jun 6, 2017

ran for i in */.meta; do cd $i; go run gen.go; cd ../../; done , success, which means we are backwards compatible (do not break generators that do NOT use property) and thus can use property only when we need to.

I need to clean up some commit history before this is suitable.

@petertseng
Copy link
Member Author

I tried #696 (map[string]interface{}) but did not find it convincing enough

@leenipper
Copy link
Contributor

we are backwards compatible (do not break generators that do NOT use property) and thus can use property only when we need to.

Very good. I had hoped that was the case.

@petertseng petertseng changed the title [Do Not Merge] clock: restore type-safety for different types of cases clock: restore type-safety for different types of cases Jun 16, 2017
@petertseng
Copy link
Member Author

OK, commits cleaned up. I did keep the three steps (explicitly call a function, explicitly call Classify, automatically call classify) since they could be useful.

@petertseng
Copy link
Member Author

and made the "remove extraneous spaces" its own commit too

Copy link
Contributor

@leenipper leenipper left a comment

Choose a reason for hiding this comment

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

Since the gen/ changes are now compatible with the existing generators, perhaps consider isolating the gen/gen.go & gen/property.go changes into their own squashed commit, and then isolating the clock/.meta changes into its own commit ?
It would make it clear what the clock changes were to employ the new approach, separate from the gen proper changes. I realize that both sets of changes go hand in hand for the new approach. I'm thinking down the road it may be handy to see an example of the net-total change for the .meta/ of an exercise to transition to the classify property approach.

Otherwise, I think it is ready to be merged.

@petertseng
Copy link
Member Author

Ah I get it. Nice idea. I'll do that. The history can be kept in its own branch; I doubt it'll ever be needed. I'll do it end of this week I hope. It is a busy week. I'm not in a terrible hurry to merge it since a generator needing it has not come up yet since the idea came up.

This allows us to specify types for each key depending on the `property`
key of a JSON object rather than having to use the least common
denominator type (which is often `interface{}`).

This change is compatible with generators that do not use this
annotation (and JSON files that have only one property will not need
it).

Discussions on this topic:
#674 (comment)
exercism/discussions#155 (comment)
The previous commit added this support to the generators, and the clock
exercise can be the first user of it.
@petertseng
Copy link
Member Author

perhaps consider isolating the gen/gen.go & gen/property.go changes into their own squashed commit, and then isolating the clock/.meta changes into its own commit ?

OK did it.

Copy link
Contributor

@leenipper leenipper left a comment

Choose a reason for hiding this comment

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

Looks good.

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.

3 participants