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

Ignore non-proto3 files #19

Closed
wants to merge 1 commit into from

Conversation

cdevienne
Copy link

In some case a proto3 file includes a proto2 file but will not use the message types in it
directly. Ignoring proto2 files instead of failing allows these cases to run smoothly.

In some case a proto3 file includes a proto2 file but will not use the message types in it
directly. Ignoring proto2 files instead of failing allows these cases to run smoothly.
@tiziano88
Copy link
Owner

Many thanks for this @cdevienne ! I am not too sure about this, since it would mean that if the binary is run against a purely proto2 directory, no output and no error will be emitted, which will definitely be confusing for users. So I would prefer not to merge this, but I am happy to discuss further, if you or anyone else has any strong opinion.

@cdevienne
Copy link
Author

Hello @tiziano88
I had the case with the protobuf file "descriptor.proto", which I think we can add to the ignored file list.
Would it be more acceptable ?

@tiziano88
Copy link
Owner

Currently the ignored list exists because for those types the proto compiler will emit "native" structs in the target language rather than proto objects. For descriptor.proto I don't think this would be the case? If a proto3 file is including descriptor.proto (which is proto2), I think the correct thing to do is to error out? Otherwise the resulting generated will probably not make sense anyways, right?

@cdevienne
Copy link
Author

Not exactly.
The descriptor file contains only things used by some extensions (like nrpc or gogo) to annotate the file, messages, fields, services and methods.
So while we need the file to load the sources, it defines nothing that will end up directly in the generated files. And as there are no proto3 version of this file (it uses proto2-only stuffs), in the elm-protobuf case the only valid behavior is to ignore it.

@tiziano88
Copy link
Owner

Well, I think what you describe is a possibility, but nothing stops a proto file from having a field of a type contained in descriptor.proto, in which case the result will be that elm-protobuf will succeed, but the resulting generated files will not compile. Anyway, I am interested to see an example of what you describe, I think in practice you are correct that in most cases it will be used for annotations only, and we can safely ignore those.

@cdevienne
Copy link
Author

Unlike what I though at first, https://github.com/protocolbuffers/protobuf/blob/master/src/google/protobuf/descriptor.proto indeed containts messages and not only extensible things for annotations.

As long as elm-protobuf does compile non-proto3 files, and since there are valid cases were a proto3 files includes the descriptor file, I think we can safely ignore it.

One possibility would be to add an option to explicitly ignore files from the command line, something like:

--elm_out="ignore=google/protobuf/descriptor.proto:."

@cdevienne
Copy link
Author

Here is an example of using descriptor.proto to define annotations:

https://github.com/nats-rpc/nrpc/blob/master/nrpc.proto

@cdevienne
Copy link
Author

#21 provides a better way to address the original problem.

@cdevienne cdevienne closed this Apr 24, 2019
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.

None yet

2 participants