feat: Add rudimentary plugin support#1
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces substantial updates to the codebase, particularly within the Changes
Tip OpenAI O1 model for chat
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (9)
- examples/projects/monorepo/packages/feature-account/codegen.yml (1 hunks)
- examples/projects/monorepo/packages/feature-account/introspection.json (1 hunks)
- internal/config.go (1 hunks)
- internal/converter.go (0 hunks)
- internal/plugins/casing.go (1 hunks)
- internal/plugins/introspection.go (1 hunks)
- internal/plugins/plugins.go (1 hunks)
- internal/plugins/typescript.go (1 hunks)
- internal/project.go (5 hunks)
Files not reviewed due to no reviewable changes (1)
- internal/converter.go
Additional context used
yamllint
examples/projects/monorepo/packages/feature-account/codegen.yml
[error] 8-8: no new line character at the end of file
(new-line-at-end-of-file)
Additional comments not posted (22)
examples/projects/monorepo/packages/feature-account/codegen.yml (1)
7-8: LGTM! The new output specification looks good.Adding the
introspection.jsonoutput using theintrospectionplugin is a great enhancement. It will generate introspection data which is very useful for tools that rely on GraphQL schema introspection.Tools
yamllint
[error] 8-8: no new line character at the end of file
(new-line-at-end-of-file)
internal/plugins/casing.go (3)
1-1: Package name change looks good!The package name has been changed from
internaltoplugins. This change is acceptable if it aligns with the overall project structure and naming conventions.
Line range hint
8-10:ToUpperfunction looks good!The
ToUpperfunction converts a string to uppercase using the standard librarystringspackage. The implementation is correct and follows best practices.
Line range hint
12-14:ToCamelfunction looks good!The
ToCamelfunction converts a string to camel case using theiancoleman/strcasepackage. The implementation is correct and follows best practices.internal/config.go (4)
11-11: Good naming choice!Renaming the
Schemafield toSchemasimproves clarity and suggests that the configuration can accommodate multiple schemas. This change enhances the readability and maintainability of the code.
14-14: Nice refactoring!Modifying the
Generatesfield from an inline struct to a separate type is a great refactoring decision. It improves code readability, maintainability, and allows for better reuse and potential extension of theGeneratesstructure in the future.
17-20: Well-structured type!Introducing the
Generatestype withPluginsandPresetfields is a great way to encapsulate related configuration options. It provides a clear and organized structure for managing the generation settings.
22-22: Consistent update!Updating the logic in the
GetConfigmethod to check forp.config.Schemasinstead ofp.config.Schemaensures consistency with the changes made to theConfigstruct. This change maintains the correct behavior of the method in light of the updated configuration structure.internal/project.go (6)
132-138: Great use of goroutines to parallelize task execution! 👍Launching a goroutine for each destination to execute the generation tasks concurrently is an efficient approach. It will help speed up the overall execution time, especially for projects with multiple destinations.
145-148: Nice work with thestrings.BuilderandExecuteDestinationTasks! 🙌Building the output in memory using
strings.Builderis a smart move. It helps optimize performance by minimizing file I/O operations.Also, extracting the task execution logic into a separate
ExecuteDestinationTasksmethod improves code readability and maintainability. Good job!
150-164: Solid file handling! 💪The code for creating the output file, writing the contents, and closing the file looks good. Handling potential errors and panicking on failures is the right approach here, as we want to halt the execution if something goes wrong with the file operations.
174-202: Excellent work on theExecuteDestinationTasksmethod! 🎉The implementation looks clean and well-structured. Here are a few highlights:
- The method signature and parameters are well-defined and appropriate for the task at hand.
- Creating a
PluginTaskstruct to encapsulate the task data is a good design choice. It keeps the method focused and avoids passing around too many parameters.- The plugin execution logic is clear and easy to follow. The use of
VerifyPluginto validate the plugin before execution is a nice touch.- The method is extensible and can easily support more plugins in the future by adding new conditions.
Overall, great job on this method! It enhances the modularity and organization of the codebase.
4-7: Appropriate import statements! 👌The new import statements look good and are necessary for the changes made in this file.
Using the project's internal
pluginspackage is the right choice here, as it keeps the code modular and encapsulated.Also, importing
slogfor logging is a great practice to improve the observability and debuggability of the codebase.
35-35: Good schema assignment! 👍Assigning the
Schemasfield from the project config to theSchemasfield of theProjectstruct is the right thing to do here. It ensures that theProjectstruct has access to the schemas defined in the config, which is necessary for further processing.The assignment is consistent with the rest of the code and maintains the integrity of the
Projectstruct.internal/plugins/typescript.go (8)
11-13: LGTM!The
Typescriptfunction is correctly invokingConvertSchemawith the appropriate parameters.
72-75: LGTM!The
AddBaseTypesfunction correctly writes the necessary base TypeScript types to the output.
77-95: LGTM!The
ConvertDefinitionfunction provides a clean and extensible way to handle different GraphQL definition kinds using a switch statement. It calls the appropriate conversion functions based on the definition kind and returns an error for unknown kinds, which is a good practice for error handling.
117-123: LGTM!The
WriteCommentfunction correctly writes the comment to the output if it exists and handles the case when the comment is empty.
125-131: LGTM!The
WriteFieldCommentfunction correctly writes the field comment to the output if it exists and handles the case when the comment is empty.
133-139: LGTM!The
WriteArgumentCommentfunction correctly writes the argument comment to the output if it exists and handles the case when the comment is empty.
215-230: LGTM!The
WriteFieldArgumentsfunction correctly writes the field arguments to the output. It effectively uses theWriteArgumentCommentandAddArgumentTypefunctions to handle argument comments and types, promoting code reuse.
232-248: LGTM!The
ConvertInputObjectfunction correctly converts the GraphQL input object definition to TypeScript syntax. It effectively uses theWriteFieldCommentandAddFieldTypefunctions to handle field comments and types, promoting code reuse.
| @@ -0,0 +1 @@ | |||
| {"message":"introspection plugin is not implemented"} | |||
There was a problem hiding this comment.
Clarify the purpose of this file and improve the message format.
It's unclear if this file is intended to be committed to the repository or if it's a temporary file.
If it's a temporary file:
- Please add it to the
.gitignorefile to prevent accidental commits.
If it's intended to be committed:
- Please provide a more descriptive message that follows a standard format for introspection output.
- Consider using a structured format like JSON or YAML for the output, with clear fields for the plugin name, version, status, and any relevant details.
| func (p* PluginTask) Introspect() { | ||
| p.Output.WriteString("{\"message\":\"introspection plugin is not implemented\"}\n") | ||
| } No newline at end of file |
There was a problem hiding this comment.
Improve the placeholder implementation.
The placeholder implementation can be improved in the following ways:
- Rename the JSON key from
messagetoMessageto follow the Go naming conventions. - Update the JSON message to be more descriptive, such as "Introspection plugin is not implemented yet."
- Return an error to indicate that the plugin is not implemented.
Apply this diff to improve the placeholder implementation:
-func (p* PluginTask) Introspect() {
- p.Output.WriteString("{\"message\":\"introspection plugin is not implemented\"}\n")
+func (p* PluginTask) Introspect() error {
+ p.Output.WriteString("{\"Message\":\"Introspection plugin is not implemented yet.\"}\n")
+ return errors.New("introspection plugin is not implemented")
}Committable suggestion was skipped due to low confidence.
| plugins: [typescript] No newline at end of file | ||
| plugins: [typescript] | ||
| 'introspection.json': | ||
| plugins: [introspection] No newline at end of file |
There was a problem hiding this comment.
Please add a new line at the end of the file.
The static analysis tool has flagged that there is no new line character at the end of the file. While this is a minor issue, it's considered a best practice to always have a new line at the end.
You can fix it by simply adding a new line at the end of the file.
Tools
yamllint
[error] 8-8: no new line character at the end of file
(new-line-at-end-of-file)
| type PluginTask struct { | ||
| Schema *ast.Schema | ||
| Output *strings.Builder | ||
| Config interface{} | ||
| } |
There was a problem hiding this comment.
Consider using a more specific type for the Config field.
The Config field is currently an empty interface, which could lead to type safety issues and make the code harder to understand and maintain.
Consider defining a specific type for the configuration, such as a struct or a map, to improve type safety and provide better documentation about the expected structure of the configuration.
| func VerifyPlugin(pluginName string, config interface{}) error { | ||
| if pluginName != "typescript" && pluginName != "introspection" { | ||
| return errors.New("unknown plugin") | ||
| } | ||
|
|
||
| return nil | ||
| } No newline at end of file |
There was a problem hiding this comment.
Improve error message and consider using a map or slice for supported plugins.
The VerifyPlugin function could be improved in a couple of ways:
- Include the plugin name in the error message for better debugging. For example:
return fmt.Errorf("unknown plugin: %s", pluginName)- Consider using a map or slice of supported plugins for better extensibility. This would make it easier to add or remove supported plugins in the future without modifying the function logic. For example:
supportedPlugins := map[string]bool{
"typescript": true,
"introspection": true,
}
if !supportedPlugins[pluginName] {
return fmt.Errorf("unknown plugin: %s", pluginName)
}| func ConvertEnum(definition *ast.Definition, output *strings.Builder) { | ||
| enumName := ToCamel(definition.Name) | ||
|
|
||
| WriteComment(definition, output) | ||
|
|
||
| output.WriteString("export enum " + enumName + " {\n") | ||
|
|
||
| for i, enumValue := range definition.EnumValues { | ||
| enumName := enumValue.Name | ||
| enumKey := ToUpper(enumValue.Name) | ||
| output.WriteString("\t" + enumKey + " = '" + enumName + "'") | ||
|
|
||
| if i != len(definition.EnumValues)-1 { | ||
| output.WriteString(",") | ||
| } | ||
| output.WriteString("\n") | ||
| } | ||
| output.WriteString("}\n") | ||
| } |
There was a problem hiding this comment.
Consider extracting comment writing logic for reusability.
The ConvertEnum function correctly converts the GraphQL enum definition to TypeScript syntax. However, consider extracting the comment writing logic (line 100) into a separate function, such as WriteComment, for better reusability across different conversion functions.
| func ConvertUnion(definition *ast.Definition, output *strings.Builder) { | ||
| unionName := ToCamel(definition.Name) | ||
|
|
||
| WriteComment(definition, output) | ||
|
|
||
| output.WriteString("export type " + unionName + " = ") | ||
| for i, alias := range definition.Types { | ||
| output.WriteString(alias) | ||
|
|
||
| if i != len(definition.Types)-1 { | ||
| output.WriteString(" | ") | ||
| } else { | ||
| output.WriteString(";") | ||
| } | ||
| } | ||
| output.WriteString("\n") | ||
| } |
There was a problem hiding this comment.
Consider extracting union types writing logic for better readability.
The ConvertUnion function correctly converts the GraphQL union definition to TypeScript syntax. However, consider extracting the logic for writing union types (lines 147-155) into a separate function, such as WriteUnionTypes, for better readability and maintainability.
| func ConvertInterface(definition *ast.Definition, output *strings.Builder, knownScalars []*ast.Definition) { | ||
| interfaceName := ToCamel(definition.Name) | ||
|
|
||
| WriteComment(definition, output) | ||
|
|
||
| output.WriteString("export type " + interfaceName + " = {\n") | ||
| for _, field := range definition.Fields { | ||
| WriteFieldComment(field, output) | ||
|
|
||
| fieldName := field.Name | ||
|
|
||
| output.WriteString("\t" + fieldName) | ||
| AddFieldType(field, output, "Maybe", knownScalars) | ||
| } | ||
|
|
||
| output.WriteString("}\n") | ||
|
|
||
| for _, field := range definition.Fields { | ||
| WriteFieldArguments(field, output, knownScalars, interfaceName) | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider extracting fields writing logic for better readability.
The ConvertInterface function correctly converts the GraphQL interface definition to TypeScript syntax. It effectively uses the WriteFieldComment and WriteFieldArguments functions to handle field comments and arguments, promoting code reuse.
However, consider extracting the logic for writing fields (lines 165-172) into a separate function, such as WriteInterfaceFields, for better readability and maintainability.
| func ConvertObject(definition *ast.Definition, output *strings.Builder, knownScalars []*ast.Definition) { | ||
| interfaceName := ToCamel(definition.Name) | ||
|
|
||
| WriteComment(definition, output) | ||
|
|
||
| output.WriteString("export type " + interfaceName + " = ") | ||
|
|
||
| // check implements | ||
| for _, implementsInterface := range definition.Interfaces { | ||
| output.WriteString(implementsInterface + " & ") | ||
| } | ||
| output.WriteString("{\n") | ||
|
|
||
| output.WriteString("\t__typename: '" + interfaceName + "';\n") | ||
| for _, field := range definition.Fields { | ||
| if field.Name == "__type" || field.Name == "__schema" { | ||
| continue | ||
| } | ||
|
|
||
| WriteFieldComment(field, output) | ||
|
|
||
| fieldName := field.Name | ||
|
|
||
| output.WriteString("\t" + fieldName) | ||
| AddFieldType(field, output, "Maybe", knownScalars) | ||
| } | ||
|
|
||
| output.WriteString("}\n") | ||
|
|
||
| for _, field := range definition.Fields { | ||
| WriteFieldArguments(field, output, knownScalars, interfaceName) | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider extracting implemented interfaces writing logic for better readability.
The ConvertObject function correctly converts the GraphQL object definition to TypeScript syntax. It effectively uses the WriteFieldComment and WriteFieldArguments functions to handle field comments and arguments, promoting code reuse.
However, consider extracting the logic for writing implemented interfaces (lines 189-191) into a separate function, such as WriteImplementedInterfaces, for better readability and maintainability.
| func AddFieldType(definition *ast.FieldDefinition, output *strings.Builder, maybeType string, knownScalars []*ast.Definition) { | ||
| isNullable := !definition.Type.NonNull | ||
|
|
||
| if isNullable { | ||
| output.WriteString("?: " + maybeType + "<") | ||
| } else { | ||
| output.WriteString(": ") | ||
| } | ||
|
|
||
| isArray := definition.Type.Elem != nil | ||
| isElemNullable := true | ||
|
|
||
| if isArray { | ||
| output.WriteString("Array<") | ||
|
|
||
| isElemNullable = !definition.Type.Elem.NonNull | ||
| } | ||
|
|
||
| if isArray && isElemNullable { | ||
| output.WriteString(maybeType + "<") | ||
| } | ||
|
|
||
| // check if scalar is known | ||
| isScalarKnown := false | ||
| for _, scalar := range knownScalars { | ||
| if scalar.Name == definition.Type.Name() { | ||
| isScalarKnown = true | ||
| } | ||
| } | ||
|
|
||
| if isScalarKnown { | ||
| output.WriteString("Scalars['" + definition.Type.Name() + "']") | ||
| } else { | ||
| output.WriteString(ToCamel(definition.Type.Name())) | ||
| } | ||
|
|
||
| if isNullable { | ||
| output.WriteString(">") | ||
| } | ||
|
|
||
| if isArray { | ||
| output.WriteString(">") | ||
| } | ||
|
|
||
| if isArray && isElemNullable { | ||
| output.WriteString(">") | ||
| } | ||
|
|
||
| output.WriteString(";\n") | ||
| } |
There was a problem hiding this comment.
Consider extracting array type handling logic for better readability.
The AddFieldType function correctly adds field type information to the output based on nullability, array type, and scalar type. It effectively uses the knownScalars parameter to handle custom scalar types.
However, consider extracting the logic for handling array types (lines 259-296) into a separate function, such as HandleArrayType, for better readability and maintainability.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores