feat!: refactor Namespace type #68
Conversation
cmwaters
requested review from
evan-forbes,
ninabarbakadze,
ramin and
renaynay
and removed request for
a team
blob/blob.go
Outdated
| return bytes.Compare(blobs[i].Namespace().Bytes(), blobs[j].Namespace().Bytes()) < 0 | ||
| ns1 := append([]byte{byte(blobs[i].NamespaceVersion)}, blobs[i].NamespaceId...) | ||
| ns2 := append([]byte{byte(blobs[j].NamespaceVersion)}, blobs[j].NamespaceId...) | ||
| return bytes.Compare(ns1, ns2) < 0 |
There was a problem hiding this comment.
Why not add method Compare(other *Blob) to Blob type instead?
There was a problem hiding this comment.
Yeah I can do that
| @@ -7,8 +7,7 @@ import ( | |||
| ) | |||
|
|
|||
| type Namespace struct { | |||
| Version uint8 | |||
| ID []byte | |||
| data []byte | |||
There was a problem hiding this comment.
One option we haven't discussed yet is making Namespace map friendly by keeping this byte slice as a string(basically string(data)). I haven't seen this being needed that often, but intuitively one would wanna map namespaces to Blobs or else sometimes.
No actionable here, but a discussion point we haven't touched previously.
… into cal/namespace-type
| return &Blob{ | ||
| NamespaceId: ns.ID, | ||
| NamespaceId: ns.ID(), |
There was a problem hiding this comment.
This is a nit but I think it should be NamespaceID
There was a problem hiding this comment.
This is the proto generated struct and I don't intend to break that but I will keep your suggestion for when we create a go-native Blob implementation
| Version: NamespaceVersionZero, | ||
| ID: append(bytes.Repeat([]byte{0x00}, NamespaceIDSize-1), lastByte), | ||
| } | ||
| return newNamespace(NamespaceVersionZero, append(bytes.Repeat([]byte{0x00}, NamespaceIDSize-1), lastByte)) |
There was a problem hiding this comment.
nit but NamespaceVersionZero vs NamespaceVersionMax ? Can we make NamespaceVersionZero --> NamespaceVersionMin?
There was a problem hiding this comment.
If we were to go back we would probably start user namespaces at v1 and have NamespaceVersionMin as 0 for the primary reserved namespaces. For now v0 is both used by external users and by us. I think it would be more confusing to call it version min
namespace/namespace.go
Outdated
| } | ||
|
|
||
| func newNamespace(version uint8, id []byte) Namespace { | ||
| data := make([]byte, 1+len(id)) |
There was a problem hiding this comment.
nit but do you want to point at NamespaceVersionSize + len(id)
There was a problem hiding this comment.
Yeah I missed these parts when going through it again
namespace/namespace.go
Outdated
| func newNamespace(version uint8, id []byte) Namespace { | ||
| data := make([]byte, 1+len(id)) | ||
| data[0] = version | ||
| copy(data[1:], id) |
There was a problem hiding this comment.
nit: NamespaceVersionIndex? for readability
namespace/namespace.go
Outdated
|
|
||
| // Version return this namespace's version | ||
| func (n Namespace) Version() uint8 { | ||
| return n.data[NamespaceVersionSize-1] |
There was a problem hiding this comment.
It was updated in two steps. I just missed this. Thanks for pointing it out
| go.work | ||
| go.work.sum |
There was a problem hiding this comment.
Do we still need this, considering #67 was merged?
There was a problem hiding this comment.
I think so, else it's going to see this as a diff between the users branch and main and will want to check it in
| func (b *Blob) Compare(other *Blob) int { | ||
| return bytes.Compare(b.RawNamespace(), other.RawNamespace()) | ||
| } |
There was a problem hiding this comment.
Usually, such methods are called Equals with the bool return value, but as long as this is not PR targeting Blob we can omit that and this method altogether
There was a problem hiding this comment.
Ref: #68 (comment)
There was a problem hiding this comment.
Thats improvement over that comment
There was a problem hiding this comment.
I actually thinking comparing is more important because it's more common to want to order these things then see if the namespaces are equal
There was a problem hiding this comment.
Of course nothing wrong with having both
Co-authored-by: Rootul P <[email protected]>
Addresses: #66