-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
feat(pkger): extend pkgs with source identifiers #18515
Conversation
ecbbe90
to
b71e264
Compare
bfd4fbe
to
51c4b35
Compare
51c4b35
to
087c97e
Compare
087c97e
to
4fb8a14
Compare
4fb8a14
to
6aedfaa
Compare
func (s *HTTPRemoteService) DryRun(ctx context.Context, orgID, userID influxdb.ID, pkg *Pkg, opts ...ApplyOptFn) (PkgImpactSummary, error) { | ||
return s.apply(ctx, orgID, pkg, true, opts...) | ||
func (s *HTTPRemoteService) DryRun(ctx context.Context, orgID, userID influxdb.ID, opts ...ApplyOptFn) (PkgImpactSummary, error) { | ||
return s.apply(ctx, orgID, true, opts...) |
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.
not a blocker for this, but a design consideration to have a think about:
boolean arguments trigger me hard because they provide very little context to what's happening. i assume true
and false
passed in to apply
have to do with dry runs, but that is a wild ass guess that required looking around at the context of these calls to even have an idea. point is, boolean arguments are a kind of hard stop on comprehension and require digging into definitions to understand.
i think it's usually worth considering how the intent of this true
vs false
value might be captured in a way that gives context to the reader. making it an enum is one way i can think of that doesn't resort to stringly typed stuff.
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 hear you, if it were exported, I'd have addressed it. However, it's internal, and don't see it necessary here.
@@ -584,6 +625,21 @@ func pkgEncoding(contentType string) Encoding { | |||
} | |||
} | |||
|
|||
func convertEncoding(ct, rawURL string) Encoding { |
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.
sorry, it's hard for me to tell but is this unit tested? it seems like a prime candidate for unit tests hitting all the branches in the switch statement
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.
not explicitly, its an internal implementation detail. Is tested in other tests that rely on this. It is purposeful that there is very limited white box testing in pkger.
a good chunk of this PR is jiggling some bits to remove the high lvl Pkg from the svc interface.
references: #18243
TODOs