-
Notifications
You must be signed in to change notification settings - Fork 510
internal/testkeys: refactor and clean up #5370
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
Conversation
Update the testkeys Keyspace interface to use uint64s for indexing keys. This is in preparation for using crrand.Perm64s for shuffling keys.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sumeerbhola reviewed 22 of 22 files at r1, 2 of 2 files at r2, 7 of 7 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @jbowens)
internal/testkeys/testkeys.go
line 477 at r3 (raw file):
// exclusive. The receiver is unmodified. func Slice(ks Keyspace, i, j uint64) Keyspace { return &sliceKeyspace{ks: ks, i: i, j: j}
should this panic if i or j are greater than ks.Count
?
internal/testkeys/testkeys.go
line 507 at r3 (raw file):
c := innerCount / n if innerCount%n > 0 { c++
This could use a comment. Something like:
// We are returning keys at index 0, n, 2n, ... up to cn (to be determined whether inclusive or exclusive), such that the last key is < innerCount. Since cn < innerCount, it becomes inclusive and we return c+1 keys (hence this increment).
Move the Slice and EveryN funcs out of the Keyspace interface and into top-level funcs that accept a Keyspace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TFTR!
Reviewable status: 16 of 23 files reviewed, 2 unresolved discussions (waiting on @sumeerbhola)
internal/testkeys/testkeys.go
line 477 at r3 (raw file):
Previously, sumeerbhola wrote…
should this panic if i or j are greater than
ks.Count
?
Done.
internal/testkeys/testkeys.go
line 507 at r3 (raw file):
Previously, sumeerbhola wrote…
This could use a comment. Something like:
// We are returning keys at index 0, n, 2n, ... up to cn (to be determined whether inclusive or exclusive), such that the last key is < innerCount. Since cn < innerCount, it becomes inclusive and we return c+1 keys (hence this increment).
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sumeerbhola reviewed 1 of 7 files at r5.
Reviewable status: 17 of 23 files reviewed, 2 unresolved discussions (waiting on @jbowens)
No description provided.