-
Notifications
You must be signed in to change notification settings - Fork 4
feat(modules): add formula cache with version comparison #91
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
Summary of ChangesHello @MeteorsLiu, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a robust caching system for module formulas, designed to optimize performance by intelligently handling formula loading and version resolution. It allows the system to quickly locate and apply the correct formula for a given module version, preventing unnecessary file I/O and parsing, and supports flexible version comparison strategies. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #91 +/- ##
==========================================
+ Coverage 68.58% 70.24% +1.66%
==========================================
Files 25 26 +1
Lines 1009 1099 +90
==========================================
+ Hits 692 772 +80
- Misses 254 259 +5
- Partials 63 68 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Code Review
This pull request introduces a classfileCache for managing formula loading and caching, with version-aware lookups. The implementation is well-structured and includes comprehensive tests. My review focuses on improving code clarity by fixing typos, addressing a critical compilation issue in the tests, and promoting better error handling and code reuse.
|
/review |
|
/review |
Code Review SummaryGood architecture with clean separation: Key findings:
See inline comments for details. |
internal/modules/dir.go
Outdated
| if err != nil { | ||
| return "", err | ||
| } | ||
| moduleDir := filepath.Join(formulaDir, escapedModPath) |
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.
what's the structure under this folder?
Implement classfileCache to manage formula loading and caching with version-aware lookups. The cache: - Maintains loaded formulas and version comparators - Supports configurable search paths with first-match-wins strategy - Parses formula files to extract version information (fromVer) - Provides version comparison using custom or GNU version ordering - Includes comprehensive test coverage for version parsing This enables efficient formula resolution across different module versions while avoiding redundant file system operations and parsing.
Add test suite for the formulaOf method in classfileCache covering: - Successful formula retrieval and loading - Formula caching behavior verification - Error handling for comparator failures - Error handling for missing formulas (FindMaxFromVer failures) - Error handling for invalid formula loading - Version resolution to formula mapping Import required packages (fmt and internal/formula) to support the new test implementations. Tests ensure proper formula caching, version matching, and error propagation in the formula lookup flow.
Redesign the module loading system with cleaner separation of concerns: - Remove classfileCache which mixed caching, loading, and version matching - Add moduleSource to manage formula repositories and handle sync - Add formulaModule to represent a single module's formula collection - moduleSource.module() handles sync and caches formulaModule instances - formulaModule.comparator() lazily loads and caches version comparator - formulaModule.at(version) handles version matching and formula loading This refactoring improves: - Clear ownership: sync in moduleSource, loading in formulaModule - Better abstraction: moduleSource -> formulaModule -> formula - Testability: each component has focused responsibilities Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Change moduleSource.module() to store sync errors in formulaModule instead of returning them immediately. Errors are now returned when comparator() or at() methods are called, allowing module creation to always succeed while still propagating sync failures at usage time. This improves error handling by: - Simplifying the module() API (no error return) - Deferring sync errors until actual module usage - Maintaining error propagation through comparator() and at() Updated tests to verify deferred error behavior and added test case for permission-denied scenarios.
Refactor parseCallArg() to use a switch statement for type assertion and explicitly reject non-string literal arguments with an error instead of silently returning an empty string. Update corresponding test to validate that non-string arguments properly return an error.
Add documentation comment explaining that if the custom comparator cannot be loaded, the module falls back to GNU version comparison. This improves code clarity by explicitly documenting the fallback mechanism in the comparator() function.
Added validation to check if fromVer is empty after parsing from the formula AST. Returns a descriptive error message when fromVer cannot be parsed, preventing silent failures and improving debugging. Also added a TODO comment to optimize max fromVer searching using a trie tree data structure for better performance.
Update test cases in source_test.go to check for error conditions using wantErr assertions instead of wantFromVer expectations. This change improves test clarity by explicitly verifying that empty source and invalid configurations correctly return errors, with minor formatting improvements for better readability.
Remove the moduleSource struct and its module caching/synchronization logic, pushing this responsibility to the caller. Simplify formulaModule to work with fs.FS already positioned at the module directory, eliminating the need for path escaping and error state management. This reduces complexity and makes the API clearer by separating concerns.
0f1cd6c to
3637607
Compare
luoliwoshang
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.
LGTM
Introduce a module-scoped formula source pipeline that resolves formulas by version from repository filesystems.
The implementation includes:
internal/modules/source.gowithformulaModule, lazycomparator()loading,at()formula caching byfromVer, andfindMaxFromVerselection of the highest applicable formula file.comparator()that usesloadComparatorFSwhen a custom comparator is available and falls back tognu.Comparewhen comparator loading fails.fromVerOf,fromVerFrom, andparseCallArgto extract and validatefromVerfrom*_llar.goxfiles, including explicit error handling for missing, empty, or non-string arguments.internal/modules/comparator.gofrom path-basedloadComparatorto filesystem-basedloadComparatorFS(fs.ReadFileFS, path), plus corresponding updates ininternal/modules/comparator_test.go.internal/modules/source_test.gosuite covering comparator behavior, version matching, caching semantics, parsing edge cases, and integration withrepo.Store.This change makes module formula resolution more deterministic and filesystem-agnostic, while improving correctness and debuggability for comparator and
fromVerparsing paths.