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

Add a "excludeFile" option #21

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

cdevienne
Copy link

Solve the case were a proto2 file is in the dependencies and we cannot remove it (and is a less-invasive solution than #19)

Example:

protoc "--elm_out=excludeFile=google/protobuf/descriptor.proto:."

This option will dynamically add files to the excludedFiles list, allowing proto2 files in the
dependencies of a proto3 file as long as its data structures are not used directly.
@cdevienne
Copy link
Author

@tiziano88 Do you consider merging this PR?

@cdevienne
Copy link
Author

@tiziano88 Sorry for poking again, but I would like to know if you intend to merge this PR.

(btw I suggest you close the PR you do not intend to merge).

@cdevienne cdevienne mentioned this pull request Apr 24, 2019
@tiziano88
Copy link
Owner

I am not particularly enthusiastic about the proposed approach, but I do think we should solve the underlying issue; I think that implementing some basic support for proto2 would actually not be too much more work, and would also be useful for people who for some reason or another are stuck with proto2. WDYT?

@cdevienne
Copy link
Author

The solution is not ideal but it is by far the simplest one solving the issue.
I think it is reasonable to use it until we have a better one (which is what I do for my project).

Basic support for proto2 could be useful indeed, but only with Bytes encoding as there is no official json mapping for proto2.

Also, implementing such a feature should be done if there is actually a clearly identified need: in my case I do not need proto2 message support.

My 2 cents ^^

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.

2 participants