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

Proto: Rename dgraph.badger.v2.pb to badgerpb2 #1314

Merged
merged 4 commits into from
Apr 21, 2020

Conversation

jarifibrahim
Copy link
Contributor

@jarifibrahim jarifibrahim commented Apr 20, 2020

This PR renames badger protobuf package from dgraph.badger.v2.pb to badgerpb2.
The pb.pb.go file has been regenerated using the pb/gen.sh script.


This change is Reviewable

Copy link
Contributor

@gja gja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ashish-goswami and @manishrjain)

@jarifibrahim
Copy link
Contributor Author

Build failed with

[11:13:17]build github.com/dgraph-io/badger/v2/badger: cannot load github.com/dgraph-io/badger/v2/pb: module github.com/dgraph-io/badger@latest found (v1.6.1), but does not contain package github.com/dgraph-io/badger/v2/pb
[11:13:17]Process exited with code 1

Looking into it.

@mvdan
Copy link
Contributor

mvdan commented Apr 20, 2020

I'm not sure I agree with this change. Protobuf package names are global and namespaced, so just badgerdb2 feels wrong. Your Go module is called github.com/dgraph-io/badger/v2 after all, not badgerv2.

@jarifibrahim
Copy link
Contributor Author

Hey @mvdan, We decided to rename the package to a shorter name after discussing it in hypermodeinc/dgraph#5216 (review)

@mvdan
Copy link
Contributor

mvdan commented Apr 20, 2020

🤷 I disagree with all three suggestions for a global namespace. I think you shoud read https://developers.google.com/protocol-buffers/docs/proto3#packages and https://developers.google.com/protocol-buffers/docs/style#packages before making a decision.

@mvdan
Copy link
Contributor

mvdan commented Apr 20, 2020

I see that dgraph uses an equally non-unique package name, too: package pb;

@jarifibrahim
Copy link
Contributor Author

Hey @gja, according to the naming convention mentioned here https://developers.google.com/protocol-buffers/docs/style#packages , the badger protobuf package name should be dgraph.badger.v2.pb. What do you think?

@jarifibrahim jarifibrahim merged commit cddf7c0 into master Apr 21, 2020
@jarifibrahim jarifibrahim deleted the ibrahim/rename-proto-package branch April 21, 2020 06:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants