-
Notifications
You must be signed in to change notification settings - Fork 54
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
imagefilter: add SupportedOutputFormats #1168
base: main
Are you sure you want to change the base?
Conversation
19a040d
to
66d1982
Compare
66d1982
to
ddda18f
Compare
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.
Thank you! I like the idea of having the SupportedOutputFormats() helper. Some suggestions for the implementation inline.
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.
Quick drive-by:
- the PR description should follow the convention:
module: change
(e.g. imagefilter: add SupportedOutputFormats) - Rebasing this on top of main and dropping the diff for the "short" output would most likely get this merged much quicker as I feel "short" output will need some more design/discussion but this here is very nice and straightforward.
392df90
to
3125747
Compare
a7a1dc6
to
192ae46
Compare
pkg/imagefilter/formatter.go
Outdated
@@ -27,20 +29,28 @@ type ResultsFormatter interface { | |||
Output(io.Writer, []Result) error | |||
} | |||
|
|||
// NewResultFormatter will create a formatter based on the given format. | |||
var supportedFormatterMap = map[string]ResultsFormatter{ |
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.
Two notes:
- We normally don't use Hungarian notation; e.g. writing the type (
Map
) in the variable name and personally I don't like it. - The behavior here is slightly different because all key lookups now return the same instance of a formatter. Since formatters don't keep state (right now) this might be OK, just wanted to write it down as it's a behavioral change I think?
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.
Thanks for the careful review. I think this is fine because currently the formatters are an empty map, once we add state the map would become a map[string]func() Foramtter
most likely to generate the new things (we could do it today but see below).
A fun fact of go is that empty structs get optimized away by the go-compiler, so at least on my system:
func TestNewDifferentObj(t *testing.T) {
fmt1, err := imagefilter.NewResultsFormatter(imagefilter.OutputFormatText)
assert.NoError(t, err)
fmt2, err := imagefilter.NewResultsFormatter(imagefilter.OutputFormatText)
assert.NoError(t, err)
assert.NotEqual(t, fmt1, fmt2)
}
is false even with the current implementation that creates a "new" formatter with every new() call.
This is a long winded way of saying, I'm okay with as it is but open for ideas.
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.
Some minor questions and bits.
I noticed the commits are now prefixed with the full package path instead of the module name. I don't know how I feel about this :) |
192ae46
to
43c0183
Compare
oh - that shouldn't happen?
should I downgrade that lib so it works with 1.22? |
pkg/imagefilter/export_test.go
Outdated
@@ -0,0 +1,3 @@ | |||
package imagefilter | |||
|
|||
var SupportedFormatterMap = supportedFormatters |
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.
SupportedFormatters
(the type is already a map
) is the only bit.
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.
grr - overlooked that one, thanks
Implements function to get a list of all supported output formats.
Add a test for the fail-case.
43c0183
to
30c82fe
Compare
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.
Thank you
Convenience function for concise help output
(This is based on #1166 and #1167)
The last commit could be split off, it's not directly related to the implementation of
SupportedOutputFormats
just extends the tests