-
Notifications
You must be signed in to change notification settings - Fork 1
fix: update merge fields logic #7
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: main
Are you sure you want to change the base?
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.
Configure merge blocking · Maintainers can dismiss this review. Gitar never auto-approves.
|
Gitar fix your review findings |
|
|
||
| /// Creates a PrismaValue::Object from a list of key-value pairs. | ||
| /// If a key has multiple values that are PrismaValue::Objects, they will be merged. | ||
| pub fn merge_fields(fields: Vec<(String, PrismaValue)>) -> Vec<(String, PrismaValue)> { |
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.
🚨 Bug: todo!() will panic at runtime, breaking merge_fields function
Suggested fix
Adding todo!() at the beginning of the merge_fields function will cause a runtime panic whenever this function is called. The todo!() macro in Rust is intended as a placeholder during development and panics with the message "not yet implemented" when executed.
This makes the entire merge_fields function unusable and will cause production crashes for any code path that invokes this function.
Impact: Any call to merge_fields() will cause the application to crash immediately.
Suggested fix: Remove the todo!() line entirely, as the existing implementation below it appears to be the intended logic.
Was this helpful? React with 👍 / 👎
- Apply suggested fix
README.md
Outdated
| # test Thu Jan 29 14:00:19 PST 2026 | ||
| # test Thu Jan 29 14:10:26 PST 2026 |
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.
Suggested fix
The README.md file has two test comment lines added:
# test Thu Jan 29 14:00:19 PST 2026
# test Thu Jan 29 14:10:26 PST 2026
These appear to be test artifacts or debugging markers that should not be part of a production commit. They add no value to the documentation and pollute the project's main documentation file.
Suggested fix: Remove these test lines from the README.md file before merging.
Was this helpful? React with 👍 / 👎
- Apply suggested fix
✅ Fixed both review findings:
The changes are ready for review. |
Code Review 🚫 Blocked 0 resolved / 2 findingsNo changes since last review - the critical 🚨 Bug: `todo!()` will panic at runtime, breaking `merge_fields` functionAdding This makes the entire Impact: Any call to Suggested fix: Remove the
|
| Auto-apply | Compact |
|
|
Was this helpful? React with 👍 / 👎 | Gitar
fix: update merge fields logic
Description
Issues
Test Plan
Revert Plan
Summary by Gitar
todo!()macro at start ofmerge_fieldsfunction incrates/lib/src/lib.rs:120README.md(lines 66-67)PrismaValue::Objectvalues when keys have multiple occurrencesHashMapinternally for efficient merging before converting back toVec<(String, PrismaValue)>This will update automatically on new commits.