Implement Cmp() functions for ID-like structs #333

Merged
dstepanov-yadro merged 3 commits from achuprov/frostfs-sdk-go:feat/add_Cmp into master 2025-02-17 15:22:55 +00:00
Member

Close

Close #294
achuprov added 3 commits 2025-02-13 16:34:27 +00:00
Signed-off-by: Alexander Chuprov <a.chuprov@yadro.com>
Signed-off-by: Alexander Chuprov <a.chuprov@yadro.com>
Signed-off-by: Alexander Chuprov <a.chuprov@yadro.com>
requested reviews from storage-services-developers, storage-core-committers, storage-core-developers, storage-services-committers 2025-02-13 16:34:27 +00:00
achuprov force-pushed feat/add_Cmp from e35d88d0f1 to 0f97a1e917 2025-02-13 16:45:36 +00:00 Compare
achuprov force-pushed feat/add_Cmp from 0f97a1e917 to af34872561 2025-02-13 17:29:10 +00:00 Compare
Author
Member

Note: Base58 lexicographic order does not match the ASCII lexicographic order.

Note: `Base58` lexicographic order does not match the `ASCII` lexicographic order.
a-savchuk reviewed 2025-02-13 20:14:10 +00:00
@ -116,0 +117,4 @@
// Cmp returns an integer comparing two base58 encoded container ID lexicographically.
// The result will be 0 if a == b, -1 if a < b, and +1 if a > b.
func (id ID) Cmp(id2 ID) int {
Member
  • How about making this function free, i. e. func Cmp(id1, id2 ID) int?
    • I think SortFunc(ids, cid.Cmp) will look better than SortFunc(ids, (cid.ID).Cmp)
    • The function documentation will become more clear, i. e. a -> id1, b -> id2
  • What about naming this function Compare similar to how it's done in the standard library?
- How about making this function free, i. e. `func Cmp(id1, id2 ID) int`? - I think `SortFunc(ids, cid.Cmp)` will look better than `SortFunc(ids, (cid.ID).Cmp)` - The function documentation will become more clear, i. e. `a` -> `id1`, `b` -> `id2` - What about naming this function `Compare` similar to how it's done in the standard library?
Author
Member

I think we should use NeoGo code style.

I think we should use [NeoGo](https://github.com/search?q=repo%3Anspcc-dev%2Fneo-go+Cmp%28&type=code) code style.
Member

Disagree but approve

Disagree but approve
@ -116,0 +118,4 @@
// Cmp returns an integer comparing two base58 encoded container ID lexicographically.
// The result will be 0 if a == b, -1 if a < b, and +1 if a > b.
func (id ID) Cmp(id2 ID) int {
return bytes.Compare([]byte(id.EncodeToString()), []byte(id2.EncodeToString()))
Member

Is there any reason to use bytes.Compare instead of strings.Compare?

Is there any reason to use `bytes.Compare` instead of `strings.Compare`?
a-savchuk marked this conversation as resolved
@ -109,0 +119,4 @@
slices.SortFunc(arr, cid.ID.Cmp)
for i := 1; i < len(arr); i++ {
if bytes.Compare([]byte(arr[i-1].EncodeToString()), []byte(arr[i].EncodeToString())) > 0 {
Member

Can you use testify library here?

Can you use `testify` library here?
a-savchuk marked this conversation as resolved
@ -109,0 +120,4 @@
for i := 1; i < len(arr); i++ {
if bytes.Compare([]byte(arr[i-1].EncodeToString()), []byte(arr[i].EncodeToString())) > 0 {
t.Fatalf("arrays are not sorted correctly")
Member

"arrayS". How many?

"arrayS". How many?
Member

Fatalf -> Fatal

`Fatalf` -> `Fatal`
a-savchuk marked this conversation as resolved
achuprov force-pushed feat/add_Cmp from af34872561 to aecf878669 2025-02-14 14:53:19 +00:00 Compare
achuprov force-pushed feat/add_Cmp from aecf878669 to a2c2db17c2 2025-02-14 14:56:16 +00:00 Compare
a-savchuk reviewed 2025-02-14 15:02:21 +00:00
@ -109,0 +119,4 @@
slices.SortFunc(arr, cid.ID.Cmp)
for i := 1; i < len(arr); i++ {
require.NotEqual(t, strings.Compare(arr[i-1].EncodeToString(), arr[i].EncodeToString()), 1, "array are not sorted correctly")
Member

array IS

array IS
a-savchuk marked this conversation as resolved
achuprov force-pushed feat/add_Cmp from a2c2db17c2 to 5d49b313be 2025-02-14 15:12:42 +00:00 Compare
a-savchuk approved these changes 2025-02-14 15:14:59 +00:00
Dismissed
dstepanov-yadro approved these changes 2025-02-17 07:32:32 +00:00
Dismissed
@ -107,2 +109,4 @@
})
}
func TestID__Cmp(t *testing.T) {

TestID__Cmp -> TestID_Cmp ?

`TestID__Cmp` -> `TestID_Cmp` ?
achuprov force-pushed feat/add_Cmp from 5d49b313be to 614b0cc480 2025-02-17 08:50:12 +00:00 Compare
achuprov dismissed a-savchuk's review 2025-02-17 08:50:13 +00:00
Reason:

New commits pushed, approval review dismissed automatically according to repository settings

achuprov dismissed dstepanov-yadro's review 2025-02-17 08:50:13 +00:00
Reason:

New commits pushed, approval review dismissed automatically according to repository settings

a-savchuk approved these changes 2025-02-17 09:04:20 +00:00
dstepanov-yadro approved these changes 2025-02-17 10:42:03 +00:00
aarifullin approved these changes 2025-02-17 15:10:24 +00:00
dstepanov-yadro merged commit c3f7378887 into master 2025-02-17 15:22:55 +00:00
Sign in to join this conversation.
No reviewers
TrueCloudLab/storage-services-developers
TrueCloudLab/storage-services-committers
No milestone
No project
No assignees
4 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-sdk-go#333
No description provided.