Feature/remote tags - #219
Conversation
|
|
dnephin
left a comment
dnephin
left a comment
There was a problem hiding this comment.
Thanks! LGTM
I think this is a good way of handling backwards compat. I left a very minor style suggestion, but I'll merge this as-is, and leave it to you if you think it's worth changing in a follow up.
| // forEachRemoteTagNoFallback runs a function for each remote tag | ||
| func (t *Task) forEachRemoteTagNoFallback(each func(string) error) error { | ||
| return t.forEachProvidedTag(each, t.config.RemoteTags) | ||
| } |
There was a problem hiding this comment.
What does NoFallback in the name mean here?
I often try to avoid functions that only call one other function, especially when there are other functions with similar names. I guess each of the callers of this function could instead directly call:
t.forEachProvidedTag(each, t.config.RemoteTags)There was a problem hiding this comment.
"NoFallback" in this case refers to the lack of "fallback" to Local tags when there are no remote tags. I generally agree on avoiding "functions that only call one other function", except when the call is (and always should be) exactly the same across multiple callers.
Initially, I did not have this function at all, and forEachTag instead called forEachRemoteTag, causing local tags to be applied twice if there were no remote tags. Using a call to forEachProvidedTag entices a later (and more forgetful) me to go back and "DRY" the code by changing it back to forEachRemoteTag 🤦♂️. I figure that this is a case of trading actually DRY code for slightly more nested depth in the codebase. Perhaps forEachRemoteTagNoFallbackToLocal would be a more clear name though?
3 participants
Closes #216