keyv - feat: moving storage setMany to use KeyvEntry#1903
Conversation
Summary of ChangesHello, 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 refactors the Highlights
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. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request refactors the KeyvEntry type to be generic and updates the setMany method signatures across various storage adapters (Dynamo, Etcd, Memcache, Mongo, Valkey) to use this new generic type. A review comment points out a type safety regression in the etcd adapter's setMany method, suggesting that KeyvEntry[] should be KeyvEntry<Value>[] to maintain type consistency. Additionally, the ttl property, now part of KeyvEntry, is currently ignored in the etcd adapter's setMany implementation.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: acc2307f8e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1903 +/- ##
=======================================
Coverage 99.88% 99.88%
=======================================
Files 37 37
Lines 3416 3420 +4
Branches 557 560 +3
=======================================
+ Hits 3412 3416 +4
Misses 4 4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@gemini /review |
There was a problem hiding this comment.
Code Review
This pull request significantly improves the type safety and consistency of the setMany method across various Keyv storage adapters. Key changes include making the KeyvEntry type generic, updating method signatures to use KeyvEntry<Value>, and refining JSDoc comments. A new test:sequential script was also added to package.json, and property access in storage/dynamo/test/test.ts was modernized. The review comments identified several type-related issues, specifically in the storage/etcd adapter where the set method's signature is overly restrictive, leading to unsafe casts in setMany and the iterator's value retrieval. In the storage/redis adapter, unsafe value as string type assertions were noted, which could cause runtime errors if non-string values are used, with a recommendation to use String(value) for safer conversion.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3fe243852d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
keyv - feat: moving storage setMany to use KeyvEntry