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

Generating path for client is transforming all relevant types to string first. #977

Merged
merged 3 commits into from Dec 16, 2016

Conversation

Projects
None yet
2 participants
@rootpd
Copy link
Contributor

rootpd commented Dec 15, 2016

I have a feeling that it could be done with a bit less duplicity within initParams. Opened to any ideas (or just push a change) :).

PR is fixing issue #961, which was partially fixed yesterday.

@rootpd

This comment has been minimized.

Copy link
Contributor Author

rootpd commented Dec 15, 2016

I see the failing Travis. Going for christmas party, will take care tomorrow (central europe time). :)

@@ -334,13 +333,50 @@ func (g *Generator) generateResourceClient(pkgDir string, res *design.ResourceDe
}
}
}
initParams := func(att *design.AttributeDefinition) []*paramData {

This comment has been minimized.

@raphael

raphael Dec 15, 2016

Member

Looks like the same logic as the one used in generateActionClient - maybe it's time to make this its own function and call it from both places?

return fmt.Sprintf({{ printf "%q" (pathTemplate .) }}, {{ pathParamNames . }})
*/}}// {{ $funcName }} computes a request path to the {{ .Route.Parent.Name }} action of {{ .Route.Parent.Parent.Name }}.
func {{ $funcName }}({{ pathParams .Route }}) string {
var params []interface{}

This comment has been minimized.

@raphael

raphael Dec 15, 2016

Member

It would be good to avoid having to create a slice for this. This method could be called multiple times per request being handled so the extra allocations / execution time could add up. Granted it is more complicated but it's possible and it seems better to take the time to do this right.

I'm thinking instead of using a temporary variable the code could use a known prefix, e.g. "param" and append an index to it. So in the loop it would be {{ toString "p" (printf "param%d" $i) $param.Attribute }} where $i is declared in the range expression ({{ range $i, $param := .Params }}). The code rendering the fmt.Sprintf would then also use the same range to build the arguments.

@raphael

This comment has been minimized.

Copy link
Member

raphael commented Dec 15, 2016

Thank you for doing this! looks great. I suggested a couple of tweaks in the comments. Have fun at the party :)

@rootpd

This comment has been minimized.

Copy link
Contributor Author

rootpd commented Dec 16, 2016

Thanks for the hints and patience! This time it even looks greener than the last time :).

@rootpd

This comment has been minimized.

Copy link
Contributor Author

rootpd commented Dec 16, 2016

Found one more bug with the order of generated params, expect one more commit. I've realized that initParams is iterating Object which is a randomized-order map. That means that also generated params are being generated in random order.

Route params cannot be generated by iterating map.
Route params need to be generated and used in Sprintf
in a correct order. As the AttributeDefinition.Type is always
a map, for route params we need to maintain this order by
other means. In this case by manufacturing AttributeDefinition
separately for each route param.
@raphael

This comment has been minimized.

Copy link
Member

raphael commented Dec 16, 2016

Thank you for the great PR!

@raphael raphael merged commit c8c5ad7 into goadesign:v1 Dec 16, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment