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 .proto to packages of Protobuf generated Java classes #700

Merged
merged 5 commits into from
May 16, 2020

Conversation

woop
Copy link
Member

@woop woop commented May 14, 2020

What this PR does / why we need it:

Adds .proto to the package of Java generated proto code in order to differentiate it from non-proto code.

Example: feast.core.CoreServiceGrpc to feast.proto.core.CoreServiceGrpc

Which issue(s) this PR fixes:

Fixes #684

Does this PR introduce a user-facing change?:

Java classes generated from Protobuf files have had .proto added to their package path.

@woop woop added the compat/breaking Breaking user-facing change label May 14, 2020
@woop woop changed the title Add .proto to package of Protobuf generated Java classes Add .proto to packages of Protobuf generated Java classes May 14, 2020
@woop woop force-pushed the adjust-proto-packages branch from 83e7649 to 39ace30 Compare May 14, 2020 02:46
@zhilingc
Copy link
Collaborator

/test test-end-to-end-batch

1 similar comment
@woop
Copy link
Member Author

woop commented May 16, 2020

/test test-end-to-end-batch

@woop woop force-pushed the adjust-proto-packages branch from 436c8b4 to 4fa3954 Compare May 16, 2020 12:26
@woop
Copy link
Member Author

woop commented May 16, 2020

/test test-end-to-end-batch

@woop woop added the lgtm label May 16, 2020
@ches
Copy link
Member

ches commented May 16, 2020

/lgtm

Hope there isn't a reason to hold this—there's no hold label—but it's potentially a big merge conflict pain waiting for anyone who branches before this gets in, so if we want it let's get it done. Anyone with already open PRs, yell at @woop not me 😉

@feast-ci-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ches, woop

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@feast-ci-bot feast-ci-bot merged commit 14cae74 into feast-dev:master May 16, 2020
@woop
Copy link
Member Author

woop commented May 16, 2020

/lgtm

Hope there isn't a reason to hold this—there's no hold label—but it's potentially a big merge conflict pain waiting for anyone who branches before this gets in, so if we want it let's get it done. Anyone with already open PRs, yell at @woop not me

I think there will be yelling, but it won't be because of this :)

We've had a long running Mexican standoff between PRs for a while now, so I have asked @mrzzy to just squash his history into a single commit (well it's 4 now) so that he doesn't lose years of his life rebasing.

Anyway, thanks for getting this merged in!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Package conflicts exist between proto and non-proto modules.
4 participants