-
Notifications
You must be signed in to change notification settings - Fork 37
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
Filter out files _before_ parsing #272
Conversation
b46a0ea
to
6fa531b
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.
This is a great refactor and fix of the ignore logic. 🙂
I had just one question and one minor suggestion, but those are in no way blocking for this PR and can be ignored if they are wrong or you disagree.
internal/io/io.go
Outdated
@@ -72,6 +72,23 @@ func JSONRoundTrip(from any, to any) error { | |||
return json.Unmarshal(bs, to) //nolint:wrapcheck | |||
} | |||
|
|||
// YAMLRoundTrip convert any value to YAML and back again. | |||
func YAMLRoundTrip(from any, to any) error { |
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.
While I am not very familiar with (un)marshalling YAML, I do not immediately see how making a roundtrip via YAML as the intermediate is any different from JSON when considering the end result. Or is there some go types that would behave differently in this case?
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.
No, you're right. This remained from a point where I considered ditching JSON altogether, but I realized later we need that for output formats. I'll have this removed before merge.
pkg/config/config.go
Outdated
level, ok := result["level"].(string) | ||
if !ok { | ||
return errLevelMustBeString | ||
} | ||
|
||
if ignore, ok := result["ignore"]; ok { | ||
var dst Ignore | ||
|
||
err := rio.YAMLRoundTrip(ignore, &dst) | ||
if err != nil { | ||
return fmt.Errorf("unmarshalling rule ignore failed: %w", err) | ||
} | ||
|
||
rule.Ignore = dst | ||
} | ||
|
||
delete(result, "enabled") | ||
|
||
rule.Level = level | ||
rule.Extra = result | ||
|
||
return nil |
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.
Probably not a big improvement, but could/should this part be shared with UnmarshalJSON
to reduce code duplication?
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.
Fair enough! Done.
Also: * Allow files to be provided either from file paths, *or* programmatically as modules. * Marshal config directly to Config struct, and prefer to use the struct rather than passing around the config as a map. Fixes #265 Signed-off-by: Anders Eknert <anders@styra.com>
6fa531b
to
bc3be5e
Compare
Also: * Allow files to be provided either from file paths, *or* programmatically as modules. * Marshal config directly to Config struct, and prefer to use the struct rather than passing around the config as a map. Fixes StyraInc#265 Signed-off-by: Anders Eknert <anders@styra.com>
Also:
Fixes #265