client: explicitly return zero-type on failures in prune functions#48713
client: explicitly return zero-type on failures in prune functions#48713thaJeztah merged 1 commit intomoby:masterfrom
Conversation
Mostly a "nit", but it makes it clearer that we're returning an empty result, and not a (partially) propagated struct. Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
added
status/2-code-review
kind/refactor
| return report, err | ||
| return image.PruneReport{}, err |
There was a problem hiding this comment.
Honestly, wish we'd used a pointer for all of these, similar to how we did for build prune; that'd allow us to just return nil
There was a problem hiding this comment.
Just for my learning, would a refactor like that be a breaking change since users are likely building against client here?
There was a problem hiding this comment.
Yeah, that's the tricky bit. Effectively ANY change in signature on an exported function is a breaking change. So if we'd adhere to Go modules and SemVer, then no change whatsoever is allowed baring (e.g.) adding a field to a struct. That sometimes is, a bit limiting; more so because some things exported were done so long time ago, before go modules enforcing SemVer (compatibility was more "through documentation"), and there were no constructs like internal/ or if there were, we at the time opted to "allow" users to use things, but "at your own risk".
So, yes, we sometimes take a somewhat "flexible" approach; e.g. we've moved some types to different packages, but when doing so move the type, add an alias for the old one, which we deprecate for one release, then remove in the next. But as we're not an actual go module (so no /v27, /v28 import paths), technically we're not allowed to do any of that.
For this one, I'm on the fence; yes, it would make a cleaner API, but it's also something that could (subtly) break people, so not 100% sure if we should make the change.
There was a problem hiding this comment.
That sometimes is, a bit limiting; more so because some things exported were done so long time ago, before go modules enforcing SemVer (compatibility was more "through documentation"), and there were no constructs like internal/ or if there were, we at the time opted to "allow" users to use things, but "at your own risk".
I'm getting a peak into Go development history from before I became a Go dev. (~1.16 😄 )
For this one, I'm on the fence; yes, it would make a cleaner API, but it's also something that could (subtly) break people, so not 100% sure if we should make the change.
Makes sense, IIUC daemon changes are easier to justify as most use cases would not be building against those packages, but client package I assume at least some Go SDK is building against. Thanks, gives me a little more understanding of the considerations here for users. :)
austinvazquez
left a comment
austinvazquez
left a comment
There was a problem hiding this comment.
Makes sense to me! :)
| return report, err | ||
| return image.PruneReport{}, err |
There was a problem hiding this comment.
Just for my learning, would a refactor like that be a breaking change since users are likely building against client here?
3 participants
Mostly a "nit", but it makes it clearer that we're returning an empty result, and not a (partially) propagated struct.
- A picture of a cute animal (not mandatory but encouraged)
🙈