-
Notifications
You must be signed in to change notification settings - Fork 0
Add tool directive #8
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
Conversation
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.
Pull request overview
This PR adds two major features to the resetgen code generator: a +resetgen comment directive for marking structs, and a -structs CLI flag for selective struct processing. The directive allows marking entire structs for Reset() generation without tagging individual fields (all exported fields are automatically processed). The -structs flag enables processing specific structs by name, supporting both simple names (e.g., User) and package-qualified names (e.g., models.User).
Key changes:
- New
+resetgendirective for struct-level marking - New
-structsCLI flag with validation for struct and package names - Package-qualified name support for disambiguating structs across packages
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| main.go | Added -structs flag parsing, validation functions (isValidGoIdentifier, isValidPackagePath), struct filtering logic, and warning system for unfound structs |
| main_test.go | Added unit tests for validation functions and shouldProcessStruct logic |
| internal/parser/parser.go | Implemented directive detection (hasResetgenDirective), updated parseStruct to handle directives and filters, added ParseSourceWithFilter function |
| internal/parser/parser_test.go | Comprehensive tests for directive parsing and struct filtering functionality |
| testdata/structs-flag/* | Test data demonstrating -structs flag usage |
| testdata/qualified-names/* | Test data for package-qualified struct names |
| testdata/directive/* | Test data showing directive usage and interaction with tags |
| README.md | Documentation for tool installation (Go 1.24+), -structs flag usage, package-qualified names, and directive syntax |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| info, err := parser.ParseFile(path) | ||
| func processFile(path string, dryRun bool, structFilter map[string]bool) (bool, error) { | ||
| // First parse to get package name | ||
| info, err := parser.ParseFile(path, nil) |
Copilot
AI
Dec 15, 2025
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.
ParseFile is called with nil as the structFilter parameter, which causes structs specified via the -structs flag to be ignored if they don't have reset tags or directives. This means the -structs flag doesn't work correctly for structs without tags or directives.
To fix this, the structFilter needs to be passed to ParseFile, but it needs to be converted to include only simple struct names (not package-qualified) that are relevant to this file's package. For example, if the filter contains "models.User" and "Config", and the current file is in the "models" package, then pass a filter containing "User" and "Config" to the parser.
| {"with dot", "github.com/user/pkg", true}, | ||
| {"nested path", "internal/api", true}, | ||
| {"complex path", "github.com/flaticols/resetgen", true}, |
Copilot
AI
Dec 15, 2025
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.
The test case expects isValidPackagePath to accept "github.com/user/pkg", but the current implementation only allows lowercase letters, digits, dots, slashes, and underscores. This would correctly validate "github.com/user/pkg", but there's a mismatch in how the function is actually used.
In the -structs flag parsing (main.go:56-77), when a qualified name like "models.User" is provided, only the first part ("models") is passed to isValidPackagePath. The function is designed to validate simple package names, not full import paths. However, this test suggests it should handle full import paths, which creates confusion about the function's intended purpose.
| **Rules:** | ||
| - Simple name (`User`) → processes ALL User structs in all packages | ||
| - Qualified name (`models.User`) → processes only User in models package | ||
| - Package path uses Go import path format (lowercase with dots/slashes) |
Copilot
AI
Dec 15, 2025
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.
The documentation states "Package path uses Go import path format (lowercase with dots/slashes)" which suggests full import paths are supported. However, the implementation in main.go (lines 59-60) only accepts qualified names with exactly one dot, splitting into two parts (package.Struct). This means full import paths like "github.com/user/pkg.Struct" are not supported.
The documentation should clarify that only simple package names are supported for qualification, not full import paths. For example: "Package name must be a simple package identifier (e.g., 'models', 'api', not full import paths)".
| - Package path uses Go import path format (lowercase with dots/slashes) | |
| - Package name must be a simple package identifier (e.g., 'models', 'api'), not a full import path (e.g., 'github.com/user/models') |
|
This PR will not be a release. I want to do some cleanup before |
Close #6