-
Notifications
You must be signed in to change notification settings - Fork 160
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
pkg/private/common: move IfIDType out of private/common and to its own package #4604
Conversation
Given that IfIDType represents generic interface IDs as used in segements distributed by the control service, it seems that it shouln't be private either, even if it currently can be. So the new package isn't private.
There was no way to convince forbidigo that our strict naming rules for interface id (which explcitly forbid lower case "ifid") that it was ok for a package name.
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.
Reviewable status: 0 of 78 files reviewed, 1 unresolved discussion (waiting on @jiceatscion and @lukedirtwalker)
pkg/segment/iface/iface.go
line 26 at r2 (raw file):
// 64 bits. There is a lot of path-type-independent code that manipulates interface IDs without // interpreting them. This type is a container for them. type IfIDType uint64
At the usage site, this now is iface.IfIDType
It stutters, is barely readable, and full of redundancy.
How about we just name it ID
? At usage site, this turns into iface.ID
which is much easier on the eyes, and contains just as much information.
Suggestion:
type ID uint64
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.
Reviewable status: 0 of 78 files reviewed, 1 unresolved discussion (waiting on @lukedirtwalker and @oncilla)
pkg/segment/iface/iface.go
line 26 at r2 (raw file):
Previously, oncilla (Dominik Roos) wrote…
At the usage site, this now is
iface.IfIDType
It stutters, is barely readable, and full of redundancy.How about we just name it
ID
? At usage site, this turns intoiface.ID
which is much easier on the eyes, and contains just as much information.
I agree this is much better. 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.
Reviewed 2 of 78 files at r1, 29 of 76 files at r2, 47 of 47 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lukedirtwalker)
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.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lukedirtwalker)
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.
Dismissed @oncilla from a discussion.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @jiceatscion)
Given that IfIDType represents generic interface IDs as used in segments distributed by the control service, it seems that it shouldn't be private either, even if it currently can be. So the new package isn't private.
Fixes #4584