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

Rename proto roachpb #2680

Merged
merged 9 commits into from
Sep 29, 2015
Merged

Rename proto roachpb #2680

merged 9 commits into from
Sep 29, 2015

Conversation

tamird
Copy link
Contributor

@tamird tamird commented Sep 27, 2015

This should go in after #2675 and #2678, should we decide to do it.

@petermattis
Copy link
Collaborator

I'm fine with renaming the proto package, but I'm not sure if I like roachpb much better, though it does avoid the conflict with the gogoproto package. Last I recall, we were trying to move stuff out of the proto package. Is that work complete? I ask because a quick inspection revealed proto/heartbeat.proto which looks like it can move to rpc.

@tamird
Copy link
Contributor Author

tamird commented Sep 27, 2015

That work isn't complete; nobody is actively working on it. I agree the name roachpb is not awesome, so I'm open to suggestions.

You're right, heartbeat.proto can move to rpc. I'll do that in another PR.

@petermattis
Copy link
Collaborator

I'm blanking on a better name that roachpb right now. But I also don't think roachpb is significantly better than proto to justify this change. I'd prefer to see us move stuff out of the proto directory and then see if what is left has a more obvious name.

@tamird
Copy link
Contributor Author

tamird commented Sep 28, 2015

It's better only in that it avoids the conflict with gogo/protobuf/proto

@tbg
Copy link
Member

tbg commented Sep 28, 2015

I don't mind roachpb (I got the impression that the pb suffix was somewhat standard, at least raft has raftpb). Of course we should still try to untangle proto but not having to manually fix up imports every now and then will be nice.

@tbg
Copy link
Member

tbg commented Sep 29, 2015

LGTM. Are you going to squash everything before you merge (since most of the intermediate commits likely don't compile/test/...) or what's the plan?

@tamird
Copy link
Contributor Author

tamird commented Sep 29, 2015

You tell me; I think it may be worth keeping as a record of work. Starting with 518bf542488e888721136d1b0b46b0e11c83cc51 the commits will compile/test

@tbg
Copy link
Member

tbg commented Sep 29, 2015

Maybe squash everything before 518bf54 then?

We always want `github.com/gogo/protobuf/proto`.
…cockroachdb/cockroach/roachpb`

manually fix rocksdb.go; gomvpkg broke it

acceptance/simulation: proto -> roachpb; gomvpkg didn't pick it up

rocksdb/db.cc: proto -> roachpb

proto files: proto -> roachpb

`go generate ./...`
replace: \bproto(\.|/)(?!Marshal|Unmarshal|Equal|Clone|Merge|Message|Int32|Uint32|Int64|Float64|Buffer|NewBuffer)(?=[A-Za-z])
with: roachpb$1
@tamird
Copy link
Contributor Author

tamird commented Sep 29, 2015

squashed; every commit should compile/test now

tamird added a commit that referenced this pull request Sep 29, 2015
@tamird tamird merged commit 7d049d7 into cockroachdb:master Sep 29, 2015
@tamird tamird deleted the rename-proto-roachpb branch September 29, 2015 12:21
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.

3 participants