-
Notifications
You must be signed in to change notification settings - Fork 3
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
refactor: limit the use of viper in packages outside cmd #70
base: main
Are you sure you want to change the base?
Conversation
Using viper outside command initialization and configuration makes it difficult to see what options gets applied. This commit refactors the initialization of the Filter struct in the filter package to use functional options over initializing from viper.
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.
Added a few comments, just one requires an answer. Otherwise, the code changes look good.
c.filter = filter.New( | ||
filter.WithMimeType(viper.GetStringSlice(flag.MimeType)), | ||
filter.WithResponseCode(viper.GetString(flag.ResponseCode)), | ||
filter.WithRecordIds(viper.GetStringSlice(flag.RecordId)), | ||
filter.WithRecordTypes(viper.GetStringSlice(flag.RecordType)), | ||
) |
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 code is identical to two other changes in this PR, could this be simplified further?
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.
Discussed it IRL: we'll improve this in a follow up PR
if i, e := strconv.Atoi(rc[0]); e == nil { | ||
f.fromStatus = i | ||
f.toStatus = i + 1 | ||
} else { | ||
panic(e) | ||
} |
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.
I know this is outside of the scope of the PR, but this looks strange to me
case "warcinfo": | ||
f.RecordTypes = f.RecordTypes | gowarc.Warcinfo | ||
case "request": | ||
f.RecordTypes = f.RecordTypes | gowarc.Request | ||
case "response": | ||
f.RecordTypes = f.RecordTypes | gowarc.Response | ||
case "metadata": | ||
f.RecordTypes = f.RecordTypes | gowarc.Metadata | ||
case "revisit": | ||
f.RecordTypes = f.RecordTypes | gowarc.Revisit | ||
case "resource": | ||
f.RecordTypes = f.RecordTypes | gowarc.Resource | ||
case "continuation": | ||
f.RecordTypes = f.RecordTypes | gowarc.Continuation | ||
case "conversion": | ||
f.RecordTypes = f.RecordTypes | gowarc.Conversion |
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.
Also outside the scope of this PR, but I think we should use stronger types that does not require bit operations.
@maeb I hope we can get this PR merged sometime soon. |
Using viper outside command initialization and configuration makes it difficult to see what options gets applied.
This commit refactors the initialization of the Filter struct in the filter package to use functional options over initializing from viper.