refactor: add builder #24
Conversation
WalkthroughThis pull request refactors the command execution and authentication flow by introducing a new builder pattern. The Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant RC as rootCmd
participant B as Builder
participant RCF as runCommandFunction
participant C as Client
participant P as Printer
U->>RC: Execute command
RC->>B: Instantiate Builder (with config, template path, variables, headers)
RC->>RCF: Call runCommandFunction(&Builder)
RCF->>B: BuildTemplate(), BuildRequest(), BuildAuth(), BuildClient(), BuildPrinter()
B-->>RCF: Return built components
RCF->>C: Execute Request
C-->>RCF: Return Response/Error
RCF->>P: Print Response/Error
Possibly related PRs
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/authentication/basic_test.go (1)
127-135: Variable naming could be improved for clarity.The variable name
templateis now used for an HTTP request object, which could be confusing sincetemplateis also used for template objects elsewhere in the code. Consider using a more descriptive name likehttpRequestorreqto clearly indicate this is an HTTP request.- template, err := http.NewRequest(http.MethodGet, "", nil) - assert.NotNil(t, template) - assert.NoError(t, err) - basic.Apply(template) - username, password, ok := template.BasicAuth() + req, err := http.NewRequest(http.MethodGet, "", nil) + assert.NotNil(t, req) + assert.NoError(t, err) + basic.Apply(req) + username, password, ok := req.BasicAuth()pkg/builder/builder.go (1)
57-60: Consider simplifying BuildPrinter methodThe method creates a temporary variable and then returns its address, which could be simplified.
func (b *Builder) BuildPrinter() *printer.Printer { - prntr := printer.NewPrinter(b.ClientConfig.PrinterConfig) - return &prntr + p := printer.NewPrinter(b.ClientConfig.PrinterConfig) + return &p }Or even more concisely:
func (b *Builder) BuildPrinter() *printer.Printer { - prntr := printer.NewPrinter(b.ClientConfig.PrinterConfig) - return &prntr + p := &printer.NewPrinter(b.ClientConfig.PrinterConfig) + return p }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
cmd/root.go(2 hunks)cmd/run.go(1 hunks)pkg/authentication/api.go(1 hunks)pkg/authentication/api_test.go(5 hunks)pkg/authentication/authentication.go(2 hunks)pkg/authentication/authentication_test.go(6 hunks)pkg/authentication/basic.go(1 hunks)pkg/authentication/basic_test.go(9 hunks)pkg/authentication/bearer.go(1 hunks)pkg/authentication/bearer_test.go(5 hunks)pkg/builder/builder.go(1 hunks)pkg/client/client.go(1 hunks)pkg/template/template.go(4 hunks)pkg/template/template_test.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (9)
pkg/authentication/authentication_test.go (1)
pkg/template/template.go (1)
Template(18-18)
pkg/authentication/authentication.go (1)
pkg/template/template.go (1)
Template(18-18)
pkg/authentication/bearer_test.go (3)
pkg/template/template.go (1)
Template(18-18)pkg/request/request.go (1)
NewRequest(94-115)internal/assert/assert.go (3)
NotNil(65-71)NoError(41-47)Equal(9-15)
pkg/template/template.go (1)
pkg/request/request.go (3)
Request(21-26)RequestData(14-19)NewRequest(94-115)
cmd/root.go (4)
pkg/builder/builder.go (1)
Builder(13-18)pkg/client/client.go (1)
ClientConfig(20-24)pkg/variables/variables.go (1)
Variables(12-12)pkg/request/request.go (1)
StringOrStringList(51-51)
pkg/authentication/bearer.go (1)
pkg/template/template.go (1)
Template(18-18)
cmd/run.go (1)
pkg/builder/builder.go (1)
Builder(13-18)
pkg/authentication/api_test.go (3)
pkg/template/template.go (1)
Template(18-18)pkg/request/request.go (1)
NewRequest(94-115)internal/assert/assert.go (3)
NotNil(65-71)NoError(41-47)Equal(9-15)
pkg/builder/builder.go (7)
cmd/root.go (2)
ClientConfig(53-53)Variables(54-54)pkg/request/request.go (2)
StringOrStringList(51-51)Request(21-26)pkg/client/client.go (2)
Client(14-18)NewClient(26-32)pkg/template/template.go (2)
Template(18-18)NewTemplate(22-31)pkg/utils/map_utils.go (1)
CombineMaps(31-38)pkg/authentication/authentication.go (2)
Auth(22-25)NewAuth(31-59)pkg/printer/printer.go (3)
Printer(23-27)NewPrinter(29-39)PrinterConfig(16-21)
🔇 Additional comments (50)
pkg/authentication/api.go (2)
8-11: Import changes look good.The updated imports properly reflect the transition from
request.Templatetotemplate.Templatewhile adding a YAML alias for improved readability.
19-19: Function signature appropriately updated.The parameter type change from
*request.Templateto*template.Templateis consistent with the builder pattern refactoring described in the PR.pkg/client/client.go (1)
8-8: Import path correctly updated.Good job updating the import path to directly use the authentication package rather than accessing it through the request package. This aligns with the architectural changes in the PR.
pkg/authentication/authentication.go (2)
8-11: Import changes look good.The updated imports properly reflect the transition from
request.Templatetotemplate.Templatewhile adding a YAML alias for improved readability.
31-31: Function signature appropriately updated.The parameter type change from
*request.Templateto*template.Templateis consistent with the builder pattern refactoring described in the PR.pkg/authentication/bearer.go (2)
8-11: Import changes look good.The updated imports properly reflect the transition from
request.Templateto*template.Templatewhile adding a YAML alias for improved readability.
18-18: Function signature appropriately updated.The parameter type change from
*request.Templateto*template.Templateis consistent with the builder pattern refactoring described in the PR.pkg/template/template_test.go (1)
1-1: Package name updated correctly.The package name has been changed from
requesttotemplateto align with the directory structure and the broader refactoring effort. This change ensures consistency with the new builder pattern implementation.pkg/authentication/basic.go (3)
8-8: Improved import with alias.Using an alias for the yaml package improves code readability and prevents potential name conflicts.
11-11: Import path updated correctly.The import has been updated from the
requestpackage to thetemplatepackage, aligning with the broader refactoring effort.
19-19: Function signature updated to use the new package.The parameter type has been changed from
*request.Templateto*template.Templateto align with the new package structure. This is consistent with the builder pattern refactoring.pkg/authentication/authentication_test.go (7)
8-8: Import path updated correctly.The import has been updated from the
requestpackage to thetemplatepackage, which is consistent with the refactoring changes.
13-16: Template instantiation updated.The template instantiation has been correctly updated to use
template.Templateinstead ofrequest.Template.
26-32: JSON template instantiation updated.The JSON template creation has been correctly updated to use
template.Template.
42-46: No-auth template instantiation updated.Template instantiation for the "no auth" test case has been correctly updated.
55-60: Basic auth template instantiation updated.Template instantiation for the basic authentication test case has been correctly updated.
70-74: Bearer auth template instantiation updated.Template instantiation for the bearer authentication test case has been correctly updated.
84-89: API auth template instantiation updated.Template instantiation for the API authentication test case has been correctly updated.
pkg/authentication/basic_test.go (8)
9-9: Import path updated correctly.The import has been updated from the
requestpackage to thetemplatepackage, aligning with the refactoring changes.
20-21: Template instantiation updated.The template instantiation has been correctly updated to use
template.Templateinstead ofrequest.Template.
38-39: JSON template instantiation updated.The JSON template creation has been correctly updated.
50-51: Invalid template instantiation updated.The invalid template instantiation has been correctly updated.
64-65: Template without auth instantiation updated.Template instantiation for the "template without auth" test case has been correctly updated.
79-80: Only username template instantiation updated.Template instantiation for the "only username" test case has been correctly updated.
95-96: Only password template instantiation updated.Template instantiation for the "only password" test case has been correctly updated.
112-113: Empty username and password template instantiation updated.Template instantiation for the "empty username and password" test case has been correctly updated.
pkg/template/template.go (4)
1-1: Good package name change to better reflect the file's purpose.The package name change from
requesttotemplatebetter aligns with the file's role - handling templates rather than requests directly.
11-11: Clean import addition for the request package.The import of the request package is now necessary as this module uses types from that package after the refactoring.
38-40: Clean method signature change to use the request package types.The Parse method now correctly returns a pointer to request.Request, and the requestData variable has been updated to use request.RequestData type. This change properly separates the template-handling logic from the request data structures.
50-50: Function call updated to use the request package.The NewRequest function call has been updated to use the request package, which is consistent with the overall refactoring of separating the template and request handling.
pkg/authentication/bearer_test.go (3)
8-8: Import correctly updated to use the template package.This change is consistent with the package restructuring in the codebase.
13-13: Template initialization updated to use the template package.The template initialization statements have been correctly updated to use template.Template instead of request.Template, aligning with the package restructuring.
Also applies to: 26-26, 42-42, 51-51
69-76: Variable name changed for better clarity.Renaming the variable from
requesttotemplateimproves readability by making it clearer what the variable represents in the context of the test.cmd/root.go (3)
13-15: Good package imports to support the builder pattern implementation.The necessary imports have been added to support the builder pattern implementation in the code.
40-47: Well-implemented builder pattern.The change from passing multiple parameters to using a
builder.Builderinstance is a good architectural improvement. This approach centralizes the parameters, making the code more maintainable, easier to read, and simpler to extend in the future.
53-58: Good variable restructuring and header configuration.The variable declarations have been reorganized for better readability, and the introduction of
defaultHeaderscentralizes header configuration. The User-Agent header is appropriately set with the application name and version.pkg/authentication/api_test.go (3)
8-8: Import correctly updated to use the template package.This change is consistent with the package restructuring in the codebase.
13-13: Template initialization updated to use the template package.The template initialization statements have been correctly updated to use template.Template instead of request.Template, aligning with the package restructuring.
Also applies to: 28-28, 46-46, 55-55
74-81: Variable name changed for better clarity.Renaming the variable from
requesttotemplateimproves readability by making it clearer what the variable represents in the context of the test.pkg/builder/builder.go (6)
13-18: Good use of the builder pattern to encapsulate configurationThe
Builderstruct cleanly consolidates all the parameters needed for request execution (client config, template path, variables, and headers), which improves modularity and reduces parameter coupling.
20-22: Good implementation of BuildClient methodSimple and straightforward wrapper around the client creation.
24-26: Good implementation of BuildTemplate methodClean delegation to the template creation function.
28-44: BuildRequest method has proper error handling and flowThe method follows a logical sequence: build template, interpolate variables, parse request, and combine headers. Error handling is appropriate at each step.
41-41: Good use of utility function for combining mapsUsing
utils.CombineMapsto merge headers from both sources is a clean approach.
46-55: BuildAuth method follows similar pattern as BuildRequestConsistent approach with proper error handling.
cmd/run.go (5)
7-7: Appropriate import for the builder packageThe import matches the new implementation pattern.
10-10: Function signature simplified with Builder patternReplacing multiple parameters with a single builder parameter significantly improves the function's interface.
11-32: Good refactoring of runCommandFunctionThe implementation now delegates creation responsibilities to the builder, making the code more maintainable and easier to follow. Error handling remains appropriate.
27-27: Simplification of error outputThe error output is now more direct and consistent.
31-32: Consistent printer initialization via builderUsing the builder to create the printer maintains consistency with the rest of the function.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
pkg/builder/builder.go (4)
13-18: Consider encapsulating these fields for controlled access.
Currently, theBuilderfields are publicly exposed. For a robust Builder pattern, you might want to keep them private and provide builder-like setters to maintain control over how these fields are initialized or modified.
24-26: Add safety checks for template path validity.
Consider handling an empty or invalidTemplatePathbefore callingtemplate.NewTemplateto avoid ambiguities or runtime errors.
28-44: Clarify header precedence when merging.
Merging headers withutils.CombineMaps(b.Headers, r.Headers)gives the request’s headers priority over the builder’s. Verify this is the intended behavior. Otherwise, reverse the argument order to ensure the builder’s headers take precedence.
46-55: Optional enforcement of authentication.
This function naturally returnsnilif the template has no auth. If your use case requires authentication to be present, consider throwing an error or providing a default no-op auth object for clarity.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/authentication/basic_test.go(9 hunks)pkg/builder/builder.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/authentication/basic_test.go
🧰 Additional context used
🧬 Code Definitions (1)
pkg/builder/builder.go (7)
cmd/root.go (2)
ClientConfig(53-53)Variables(54-54)pkg/request/request.go (2)
StringOrStringList(51-51)Request(21-26)pkg/client/client.go (2)
Client(14-18)NewClient(26-32)pkg/template/template.go (2)
Template(18-18)NewTemplate(22-31)pkg/utils/map_utils.go (1)
CombineMaps(31-38)pkg/authentication/authentication.go (2)
Auth(22-25)NewAuth(31-59)pkg/printer/printer.go (3)
Printer(23-27)NewPrinter(29-39)PrinterConfig(16-21)
🔇 Additional comments (2)
pkg/builder/builder.go (2)
20-22: Straightforward client creation.
This function cleanly instantiates a new client with the provided config.
57-60: Printer creation looks good.
The code succinctly constructs a printer instance with the correct configuration.
Summary by CodeRabbit
New Features
Refactor
Builderstruct to encapsulate configuration for requests, improving modularity and usability.