feat(unit): add InstanceUnit helpers #475
Conversation
|
Small step towards templated units; going with the "small reviewable chunks" approach instead of the usual megareview. |
|
@jonboulle Assigned #303 to you. |
|
Oops I thought it already was. |
|
|
||
| // IsInstanceUnit determines whether the given unit name appears to be an instance | ||
| // of a template unit. If so, it returns a *InstanceUnit and true; otherwise, nil and false. | ||
| func IsInstanceUnit(name string) (*InstanceUnit, bool) { |
There was a problem hiding this comment.
It seems like you could just simplify this down to
func InstanceUnit(name string) *InstanceUnit {
...
}
The non-nilness of the first return value encompasses the truthiness of the second.
There was a problem hiding this comment.
Dammit, Waldo! I originally had that but then kept thinking back to the exchange we had recently about inferring truthiness, and so eventually ended up here.
Would you object to func MaybeInstanceUnit(name string) *InstanceUnit ?
There was a problem hiding this comment.
What does the 'maybe' get you in this situation? You should check for a nil InstanceUnit, even if the code promises to always return a non-nil pointer. I like the usage of 'maybe' when there is no way to tell whether or not a function will *do something, but not for a return value.
There was a problem hiding this comment.
// MaybeInstanceUnit takes a unit name and possibly returns an InstanceUnit, depending on
// whether the unit name matches the pattern expected of an instance unit. If not, it returns nil.
Idk, I'm not sold on the conflation between func InstanceUnit and type InstanceUnit
There was a problem hiding this comment.
What about FindInstanceUnit?
There was a problem hiding this comment.
Counter-proposal: UnitNameToInstance? (inspired by systemd once again)
|
Renamed, simplified. |
| } | ||
|
|
||
| // UnitNameToInstance determines whether the given unit name appears to be an instance | ||
| // of a template unit. If so, it returns an *InstanceUnit; otherwise, nil. |
There was a problem hiding this comment.
Time for nit-pickery: nil is a valid value of *InstanceUnit, so this could be reworded to:
If so, it returns a non-nil *InstanceUnit; otherwise, nil.
There was a problem hiding this comment.
<3 come back soon!!
|
lgtm w/ one nit |
feat(unit): add InstanceUnit helpers
No description provided.