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

Inconsistent setField method in generated examples #78

Closed
MHDante opened this issue May 28, 2018 · 4 comments
Closed

Inconsistent setField method in generated examples #78

MHDante opened this issue May 28, 2018 · 4 comments

Comments

@MHDante
Copy link

MHDante commented May 28, 2018

A question about the examples/generated files.

in #67, @jonnyreeves commited updated the generated files when commiting, resulting in the following change:

https://github.com/improbable-eng/ts-protoc-gen/pull/67/files#diff-6cee6eac2b0bae17197dc838b7af57a2L208

I'm trying to set up development on my local environment, and the most recent version of protoc (3.5.1) is generating js that uses the setProto3*Field methods (e.g. setProto3EnumField)

I asume that perhaps @jonnyreeves' protoc is out of date, or is there a way to generate the js file using the generic setField method?

@jonnyreeves
Copy link
Contributor

Ah yes, sorry about that. I think we need to enforce a given version of protoc for consistency. Two options I see are:

  1. Add a CI build step which regenerates the example protos and then checks for changes, if there are any it will fail the CI build with a descriptive error message

  2. Add a local build step which checks the users locally installed version of or protoc.

However I'm welcome to other suggestions 😀

@MHDante
Copy link
Author

MHDante commented May 28, 2018

2 seems to be a problem as protoc is usually installed globally. A server developed on the same machine may require a newer version.

1 is nice, as it serves as a bit of a unit test against unwanted regressions, but I assume most changes to the code would cause output to differ, and CI tests that fail always are a bit annoying.

You could also add the version to Contributing.md and manually note unwarranted changes to the generated output when reviewing PRs

@jonnyreeves
Copy link
Contributor

Oh yes, both of these are flawed, probably not a great idea to respond to GitHub issues at 7am 😂

You're right, updating the contribution guide is an obvious first step, however another spin on option #2 may be to download protoc as part of generate.sh as it is a standalone binary

@easyCZ
Copy link
Contributor

easyCZ commented May 28, 2018

I'm leaning towards a pinned version of protoc for the repository. This makes the experience consistent for all contributors. Furthermore, an upgrade is only a PR away which helps us track the protoc target we're building against.

I can see three ways to get this done:

  1. Docker container with protoc
  2. Use grpc-tools which bundles protoc but hasn't been updated in a while
  3. Use Bazel to generate the examples. This would require contributors to install bazel.

I like 1 and 3 or going with downloading protoc

easyCZ pushed a commit to easyCZ/ts-protoc-gen that referenced this issue May 28, 2018
jonnyreeves pushed a commit that referenced this issue May 29, 2018
* Download protoc when generating protos. Fixes #78

* Fix bash

* Improve message

* Use a variable for version of protoc

* Remove install-protobuf

* CR
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

No branches or pull requests

3 participants