-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
feat(goctl): add filename annotation to group related API routes for … #5374
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
kevwan
left a comment
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.
Thanks for this feature! The filename annotation solves a real pain point with file proliferation in large APIs. The test data file (filename_demo.api) is excellent for demonstrating the use cases.
However, there are two important issues that need to be addressed before merging:
1. Missing Unit Tests
The PR lacks unit tests for the new aggregation logic. Please add tests covering:
- Multiple routes with same filename - Verify they merge into one file correctly
- Mixed scenarios - Some routes with
filenameannotation, some without - Import deduplication - Test that duplicate imports are properly handled
- Edge cases:
- Routes in different groups with same filename
- SSE routes with filename annotation
- Invalid filename values (special characters, path separators)
Suggested test file: tools/goctl/api/gogen/genhandlers_test.go and genlogic_test.go
2. Import Deduplication Implementation
The current string-based import deduplication (lines 74-91 in genhandlers.go and similar in genlogic.go) has potential issues:
for _, imp := range strings.Split(importsStr, "\n\t") {
imp = strings.TrimSpace(imp)
// ... string comparison
}Problems:
- Fragile parsing - depends on exact formatting from
genHandlerImports() - Won't handle different import styles (e.g.,
"fmt"vsfmt "fmt"vsf "fmt") - Could fail with multi-line imports or comments
Suggestion: Consider using a map-based approach or Go's go/parser package for more robust deduplication:
importMap := make(map[string]bool)
for _, imp := range strings.Split(importsStr, "\n\t") {
imp = strings.TrimSpace(imp)
if len(imp) > 0 && !importMap[imp] {
importMap[imp] = true
f.imports = append(f.imports, imp)
}
}Or better yet, parse the import statements properly to handle aliased imports correctly.
Once these are addressed, this will be a great addition to goctl! 🚀
kevwan
left a comment
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.
One more suggestion:
3. Use English in Demo Code
The test file filename_demo.api contains Chinese comments:
// Product - 商品资源
ProductReq {
ProductId int64 `path:"productId"`
}
// Order - 订单流程
CreateOrderReq {
...
}Please use English for all comments, type names, and documentation in the demo file. go-zero has an international user base, and English code examples ensure all contributors can understand and learn from them.
Suggested changes:
商品资源→Product Resource订单流程→Order Workflow用户认证→User Authentication- etc.
This aligns with the project's convention of maintaining English-first documentation for better international collaboration. 🌍
…generating api service code
…enhandlers.go and genlogic.go
|
@kevwan Thank you for the detailed review and valuable feedback! 🙏 I've addressed all the issues you mentioned: 1. ✅ Unit Tests AddedI've created comprehensive test coverage for the new aggregation logic:
All tests pass successfully ✅ 2. ✅ Import Deduplication ImprovedReplaced the string-based approach with a more robust map-based implementation: importMap := make(map[string]bool)
for _, existing := range f.imports {
importMap[existing] = true
}
for _, imp := range strings.Split(importsStr, "\n\t") {
imp = strings.TrimSpace(imp)
if len(imp) > 0 && !importMap[imp] {
importMap[imp] = true
f.imports = append(f.imports, imp)
}
}This approach is more efficient (O(n) vs O(n²)) and handles edge cases better. 3. ✅ Demo File Now in EnglishAll Chinese comments in
The demo file is now fully accessible to the international community! 🌍 Thank you again for taking the time to review this PR thoroughly. The feedback has significantly improved the code quality and test coverage. Looking forward to your review of the updates! 🚀 |
Feature: Add
filenameannotation to group API routes for generating api service codeWhat
filenameannotation in@serverblockfilenameis not specifiedWhy
How
filenamePropertyconstant to identify the annotationgenHandlersto aggregate routes by(subdir, filename)keygenLogicto aggregate routes by(subdir, filename)keyExample
See
api/gogen/testdata/filename_demo.apifor complete usage examples.Testing
Expected result:
filenamemerge into one filefilenamegenerate separately (traditional behavior)