-
Notifications
You must be signed in to change notification settings - Fork 157
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
Converted to mod, using jsonpb to marshal stub responses and fixed tests #62
Conversation
Thankyou for this well written PR 👍 . Sorry for the late response, I will try to find a time this week to detailed check of this PR. |
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.
In the main()
@generator.go, what you want to introduce there?
what is the benefit of using pluginpb.codeGeneratorRequest
instead of generator.Request
?
does the old lib is deprecated? because the updated code seems less simplistic than previous one.
} | ||
|
||
gen.CommandLineParameters(gen.Request.GetParameter()) | ||
params := make(map[string]string) |
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.
previously we have this one line gen.CommandLineParameters(gen.Request.GetParameter())
and now we had to manually parse the parameter, care to elaborate?
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,
The reason for the change is that the original generator lib is deprecated, and is not covered by the protocol buffer compatibility guarantee anymore (more details here).
I think there may have been some issue using generator with protogen as well (I can't remember). I'm happy to have a look at backing this part out if its a deal breaker
Thanks for having a look at my PR :)
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.
Thankyou for your detailed explanation. PR accepted :)
I've updated the master branch on my fork with these changes:
https://github.com/hairymike/gripmock
And created a docker image:
https://hub.docker.com/r/hairymike/gripmock
First of all, great work on
gripmock
!It would be great to start upgrading some of the other libraries in gripmock. This PR would lay some of the groundwork for this by first changing over
go modules
, and replacingpackr2
withpkger
.There was a problem running
go mod init
when usingpackr2
, so I went ahead and migrated it.Included in this PR:
well_known_type
test failure. The cause of the issue was that the keywordtype
was making it into an import alias in theserver.tmpl
file.protogen
Few issues that this might resolve:
#61
#58
#51