-
Notifications
You must be signed in to change notification settings - Fork 2
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
refactor: GetURL() -> GetLocator() #22
Conversation
machinery/types.go
Outdated
@@ -15,10 +15,10 @@ type Object interface { | |||
|
|||
GetNamespace() string | |||
GetName() string | |||
GetURL() string | |||
GetIdentity() string |
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.
An alternative that might suit also is GetRef()/GetReference()
that aligns with the targetRef
concept 🤔
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.
I thought about something along the lines of "reference" too. Maybe ReferentId
or ReferentIdentifier
or yet ReferentUniqueId
, ReferentUID
, etc. It makes sense for objects that are meant to be linked to others in the topology (from a targetRef
or from any kind of reference really).
But truth is this is about uniquely identifying the objects before being about linking them. I suppose a more generic term that captures this idea, that is extensible to unlinked objects too, is preferable. It bothers me though that "UID" is taken and we cannot use it 😕
"Identity" is good. I don't love it though. IDK why. Maybe because it's too close to UID and we don't want people to get confused about these two.
Other options that crossed to mind include Handle
and Locator
.
Either "identity", "handle" or "locator" (or anything along these lines really), another open question I have regards to qualifying the term:
- "unique", to explicit this constraint
- "topology", to denote purpose – though arguably it can be used to other purposes too
- "descriptive" or "semantic", in contrast to "opaque" (like Kube's UID)
So, any combination of these could work. E.g. "TopologyUniqueId", "DescriptiveHandle", "UniqueSemanticLocator" 🤯
Anyway... Right now I'm kinda leaning to "locator". But I also don't know why. WDYT?
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.
Hmm, yeah Identity
sounds closely related to the kube UID
🤔 I like Locator
or UniqueLocator
if we are choosing a combination 👍
@Boomatang Do you have any thoughts on this?
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.
I don't have that strong of an opinion on the naming. The term to Locator
sounds good, it gives a good indication of what the resulting function return should be used for.
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.
Updated GetUrl()
to GetLocator()
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.
I am happy with these changes.
Signed-off-by: KevFan <[email protected]>
Signed-off-by: KevFan <[email protected]>
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.
🏅
Description
Closes: #10
Rename
GetURL()
toGetIdentity()
GetLocator()
as it broad and intuitively suggests that the method returns a unique identifier for the object. It doesn’t carry any specific technical connotations (like URL or UID) that might mislead users. It also aligns well with the idea of uniquely identifying an object within the Policy Machinery topology, regardless of whether it is a cluster object or an in-memory object without implying a specific formatAn alternative that might suit also is
GetRef()/GetReference()
that aligns with thetargetRef
concept.