feat: auto-generate ServiceResourcePath constants (#75)#83
Conversation
…r strings Fixes #75 - extends gen-imports to generate constants.go in each resource package with ServiceResourcePath constant, updates all 163 DAO files to use it instead of hardcoded strings.
- Add unit tests for GenerateConstantsFile, GetServiceDisplayName, GroupByService, GetPackageInfo functions - Add build verification step to gen-imports script to catch compilation errors early
- Rename custom/cfn/ to custom/cloudformation/ (AWS CLI convention) - Add registry.DefaultDisplayNames() as single source of truth - Remove duplicate ServiceDisplayNames map from genimports (-70 lines) - genimports now imports registry for display name lookup
… coverage - Fix vpc/transit-gateways BaseDAO: ec2 -> vpc (match ServiceResourcePath) - Apply strings.CutPrefix modernization in readPackageName - Add comprehensive tests for FindRegisterPackages, GetProjectRoot, readPackageName - Improve genimports test coverage: 48.3% -> 94.8%
This comment was marked as resolved.
This comment was marked as resolved.
PR Review: Auto-generate ServiceResourcePath constants (#75)SummaryThis PR implements automatic generation of ✅ Strengths1. Excellent Code Quality
2. Outstanding Test Coverage
3. Build Verification
4. Documentation
5. Consistency Improvements
🔍 Code Review Detailsgenimports.goGood practices:
Minor observation:
transit-gateways fixCritical bug fix:
🎯 Potential Issues & Suggestions1. Test Case: Package Name with Comments (Minor)Location: The test "handles package with trailing content" expects Suggestion: func readPackageName(filePath string) string {
// ... existing code ...
for scanner.Scan() {
line := strings.TrimSpace(scanner.Text())
if pkg, ok := strings.CutPrefix(line, "package "); ok {
// Strip comments
if idx := strings.Index(pkg, "//"); idx >= 0 {
pkg = strings.TrimSpace(pkg[:idx])
}
return pkg
}
}
return ""
}Impact: Low - Real register.go files are unlikely to have inline comments after package declarations 2. Error Handling in FindRegisterPackages (Low)Location: The function returns early on any error during filepath.Walk, which could fail silently if a single file has permission issues. Suggestion: Consider logging warnings for individual file errors while continuing to walk the directory tree, only returning error for critical failures. Impact: Low - Unlikely in normal development environments 3. Missing Test: Integration Test (Enhancement)The test suite has excellent unit test coverage but lacks an integration test that:
Suggestion: Add an integration test in Impact: Low - Current CI verification provides similar protection 4. Generated File Headers (Minor - Enhancement)Location: The generated constants.go files include "To regenerate: task gen-imports" but the actual command is Suggestion: Update comment to: // To regenerate: task gen-imports (or: go generate ./...)Impact: Very Low - Documentation clarity only 🔒 Security Considerations✅ No security concerns identified
⚡ Performance Considerations✅ Good performance characteristics
📊 Test CoverageExcellent coverage:
Minor gap:
🎨 Best Practices Adherence✅ Follows Go best practices:
✅ Follows project conventions:
📝 Documentation Quality✅ Excellent documentation:
🔄 Refactoring QualityThe cfn → cloudformation rename is well-executed:
Final Verdict✅ APPROVE with minor suggestionsThis is a high-quality PR that:
The minor suggestions are optional enhancements that don't block merging:
No blocking issues identified. The code is production-ready. Metrics
Great work! 🎉 |
1 participant
Summary
ServiceResourcePathconstants in each resource package for consistent error messagescustom/cfntocustom/cloudformationto match AWS CLI naming conventionsec2→vpc)Changes
ServiceResourcePath = "service/resource"Closes #75