api/types/container: create type for changes endpoint#45353
api/types/container: create type for changes endpoint#45353thaJeztah merged 2 commits intomoby:masterfrom
Conversation
520c5f8 to
de63510
Compare
| case ChangeDelete: | ||
| return "D" | ||
| default: | ||
| return "" |
There was a problem hiding this comment.
Should we do something different here to make it more obvious when it's an unknown value?
There was a problem hiding this comment.
I was considering that; for now I stuck with what the CLI already used; #45235 (comment)
And wasn't sure what else to put here ("-" ? "?" ? <unknown> ?) WDYT?
There was a problem hiding this comment.
I personally like something really really verbose that'll definitely screw up formatting like <unknown> or <invalid> because it increases the chances that someone notices (because it's definitely an error somewhere if we run into that case) 😅
There was a problem hiding this comment.
I guess the alternative could be to
- not implement the stringer interface, and instead make this a
Change()with a(string, error)return. - return
strconv.Itoa()to return the numeric value
I do like the stringer interface though (is handy to have), and if we return an error, we also need to take into account "what if we add more options in future?" So yeah, perhaps <unknown> is the way to go 🤔
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
- use gotest.tools for asserting - check result returned Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
de63510 to
66cf0e3
Compare
|
Fixed the test, and pushed a commit to slightly improve that test; https://github.com/moby/moby/compare/de63510d70071a034b03b79934c7e70fa294a9ef..66cf0e3f5575cace57219607d042cc1c4cd536d2 |
|
Thanks! I'll bring this one in, and will do a small follow-up to discuss/make the |
archive.Changesfrom the API #45235- What I did
- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)