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

Update generator.go #164

Closed
wants to merge 4 commits into from
Closed

Update generator.go #164

wants to merge 4 commits into from

Conversation

borovan-zz
Copy link

Hi, sorry this is my first pull request ever so go easy!

There was a breaking change in satori/go.uuid#66 satori UUID. Goa fixed this, but the models that Gorma generates have the following line -

model.ID = uuid.NewV4()

which generates the error...

models\character.go:121:23: multiple-value uuid.NewV4() in single-value context

So if you use the fixed version (goa/uuid) with the workaround as opposed to directly including satori/go.uuid then it compiles. Probably want to test it though!

@raphael
Copy link
Member

raphael commented Apr 30, 2018

Thank you for the PR! Seems the "better" fix would be to change the generated code to produce the right call. So updating

{{ if eq $pk.Datatype "uuid" }}model.{{$pk.FieldName}} = uuid.NewV4(){{ end }}
to use uuid.Must(uuid.NewV4()) instead. Also as you mentioned bonus points if there was a test :)
Finally would you mind updating the .travis.yaml file so that CI runs against newer version of Go (thinking 1.9.x and 1.10.x) since goa isn't compatible with 1.7.x anymore.

@borovan-zz
Copy link
Author

Hope that works. Sorry I'm doing a bit of a intensive Go training course right now and tests are later this week! Haven't written one ever, probably why my code always breaks.

@raphael
Copy link
Member

raphael commented May 24, 2018

Apologies, I lost track of this PR. This is getting addressed by #166 which also updates the Travis "on" section.

@raphael raphael closed this May 24, 2018
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