Temporals #115
Temporals #115
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds chrono-backed temporal support (DateTime, Date, Time, Duration) to parser and value layers, including ISO-like duration string parsing and a new ParseTemporalError; bumps Cargo edition/dependencies; many import/style reorderings, expanded tests, and multiple visibility/attribute doc edits. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Redis
participant Parser
participant Chrono
participant FalkorValue
participant Consumer
Redis->>Parser: Return typed value (marker + payload)
Parser->>Parser: Match ParserTypeMarker (including DateTime/Date/Time/Duration)
alt Timestamp (integer) path
Parser->>Chrono: From seconds -> DateTime / NaiveDate / NaiveTime
Chrono-->>Parser: Temporal value or error
else Duration (string or int) path
Parser->>Parser: parse_duration_from_string("P...") or seconds -> chrono::Duration
Parser-->>Parser: Validate & convert units
end
Parser->>FalkorValue: Wrap as temporal variant
FalkorValue-->>Consumer: Return typed FalkorValue
alt parse error
Parser-->>Consumer: ParseTemporalError
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks (2 passed, 1 inconclusive)❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
Poem
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull Request Overview
Adds support for temporal types (DateTime, Date, Time) to the client’s value model, parser, and user API, bumps the Rust edition, and reapplies import and formatting cleanup across many modules.
- Introduce new
FalkorValuevariants and accessors (as_date_time,as_date,as_time). - Extend
ParserTypeMarkerandparse_typeto handle Unix‐timestamp‐based temporal parsing. - Update project to Rust 2024 edition, add
chronodependency, and standardize import/format ordering.
Reviewed Changes
Copilot reviewed 26 out of 28 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/value/mod.rs | Defined DateTime, Date, Time variants and corresponding as_* methods. |
| src/parser/mod.rs | Added DateTime, Date, Time markers and parsing branches, imported chrono. |
| Cargo.toml | Bumped edition to 2024 and declared chrono = "0.4.41". |
Multiple src/* files |
Reordered imports and updated format! calls to use named‐argument syntax. |
Comments suppressed due to low confidence (1)
src/graph/query_builder.rs:267
- The call
format!("${e}")uses a named placeholderewithout providing it as an argument. You can revert toformat!("${}", e)or explicitly name the argument:format!("${e}", e = e).
.map(|e| format!("${e}"))
| DateTime::<Utc>::from_timestamp(redis_value_as_int(val)?, 0).ok_or( | ||
| FalkorDBError::ParseTemporalError( | ||
| "Could not parse date time from timestamp".to_string(), | ||
| ), | ||
| )?, | ||
| ), | ||
| ParserTypeMarker::Date => FalkorValue::Date( | ||
| DateTime::<Utc>::from_timestamp(redis_value_as_int(val)?, 0) | ||
| .map(|dt| dt.date_naive()) | ||
| .ok_or(FalkorDBError::ParseTemporalError( | ||
| "Could not parse date from timestamp".to_string(), | ||
| ))?, | ||
| ), | ||
| ParserTypeMarker::Time => FalkorValue::Time( | ||
| DateTime::<Utc>::from_timestamp(redis_value_as_int(val)?, 0) | ||
| .map(|dt| dt.time()) | ||
| .ok_or(FalkorDBError::ParseTemporalError( | ||
| "Could not parse time from timestamp".to_string(), | ||
| ))?, |
There was a problem hiding this comment.
Using DateTime::<Utc>::from_timestamp(...).ok_or(...) is incorrect because from_timestamp returns a DateTime<Utc> directly, not an Option. Consider using Utc.timestamp_opt(secs, nsecs) (which yields a LocalResult) or the safe construction APIs, then map failures to FalkorDBError::ParseTemporalError.
| DateTime::<Utc>::from_timestamp(redis_value_as_int(val)?, 0).ok_or( | |
| FalkorDBError::ParseTemporalError( | |
| "Could not parse date time from timestamp".to_string(), | |
| ), | |
| )?, | |
| ), | |
| ParserTypeMarker::Date => FalkorValue::Date( | |
| DateTime::<Utc>::from_timestamp(redis_value_as_int(val)?, 0) | |
| .map(|dt| dt.date_naive()) | |
| .ok_or(FalkorDBError::ParseTemporalError( | |
| "Could not parse date from timestamp".to_string(), | |
| ))?, | |
| ), | |
| ParserTypeMarker::Time => FalkorValue::Time( | |
| DateTime::<Utc>::from_timestamp(redis_value_as_int(val)?, 0) | |
| .map(|dt| dt.time()) | |
| .ok_or(FalkorDBError::ParseTemporalError( | |
| "Could not parse time from timestamp".to_string(), | |
| ))?, | |
| Utc.timestamp_opt(redis_value_as_int(val)?, 0) | |
| .map_err(|_| FalkorDBError::ParseTemporalError( | |
| "Could not parse date time from timestamp".to_string(), | |
| ))? | |
| ), | |
| ParserTypeMarker::Date => FalkorValue::Date( | |
| Utc.timestamp_opt(redis_value_as_int(val)?, 0) | |
| .map_err(|_| FalkorDBError::ParseTemporalError( | |
| "Could not parse date from timestamp".to_string(), | |
| ))? | |
| .date_naive() | |
| ), | |
| ParserTypeMarker::Time => FalkorValue::Time( | |
| Utc.timestamp_opt(redis_value_as_int(val)?, 0) | |
| .map_err(|_| FalkorDBError::ParseTemporalError( | |
| "Could not parse time from timestamp".to_string(), | |
| ))? | |
| .time() |
There was a problem hiding this comment.
you are wrong pub const fn from_timestamp(secs: i64, nsecs: u32) -> Option
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
.gitignore (1)
8-8: Use a trailing slash for directory ignoresFor consistency with the other directory entries above (
.idea/,.vscode/,.vs/), append a trailing slash so only the directory is ignored and not any file that might coincidentally be named.history.-.history +.history/src/value/path.rs (1)
6-6: Minor style nit: keep import groups consistently ordered
parser::redis_value_as_vecis now tacked on to the end of an otherwise type-centric list. Consider either:
- Alphabetical ordering, or
- Grouping parser helpers in a dedicated
parser::{ … }blockto keep large
use crate::{ … }statements quickly scannable.src/client/builder.rs (1)
150-155: Preferexpectoverunwrap()in tests for clearer failures
unwrap()will panic without context if the build fails. Anexpect("sync builder failed")makes the root cause obvious when a test breaks.- assert!( - FalkorClientBuilder::new() - .with_connection_info(connection_info.unwrap()) - .build() - .is_ok() - ); + assert!( + FalkorClientBuilder::new() + .with_connection_info(connection_info.expect("invalid connection_info")) + .build() + .is_ok(), + "sync builder failed", + );src/response/index.rs (2)
41-55: Avoid the extraparse_typeround-trip inparse_string_array
parse_type(ParserTypeMarker::Array, ..)first revalidates the type marker and then constructs a fullFalkorValueonly to immediately tear it down withinto_vec().
The same result can be achieved more cheaply by callingredis_value_as_veconvaland mapping the elements, saving one allocation and a non-trivial match cascade.- let vector = parse_type(ParserTypeMarker::Array, val, graph_schema)?.into_vec()?; - vector - .into_iter() - .map(FalkorValue::into_string) - .collect::<Result<Vec<String>, FalkorDBError>>() + redis_value_as_vec(val)? + .into_iter() + .map(redis_value_as_string) + .collect()This keeps the error surface identical while shaving a bit of work off the hot path.
121-137: Minor: duplicate magic numberThe literal
9appears twice (array size in the pattern and in the error message).
Consider capturing it in a constant to keep the two in sync:const INDEX_FIELDS: usize = 9; … let fields_arr: [redis::Value; INDEX_FIELDS] = …?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
.gitignore(1 hunks)Cargo.toml(2 hunks)src/client/asynchronous.rs(2 hunks)src/client/blocking.rs(7 hunks)src/client/builder.rs(2 hunks)src/client/mod.rs(1 hunks)src/connection/asynchronous.rs(1 hunks)src/connection/blocking.rs(1 hunks)src/error/mod.rs(3 hunks)src/graph/asynchronous.rs(2 hunks)src/graph/blocking.rs(2 hunks)src/graph/mod.rs(4 hunks)src/graph/query_builder.rs(3 hunks)src/graph_schema/mod.rs(2 hunks)src/lib.rs(1 hunks)src/parser/mod.rs(5 hunks)src/response/constraint.rs(2 hunks)src/response/execution_plan.rs(1 hunks)src/response/index.rs(2 hunks)src/response/lazy_result_set.rs(1 hunks)src/response/mod.rs(1 hunks)src/response/slowlog_entry.rs(1 hunks)src/value/graph_entities.rs(2 hunks)src/value/mod.rs(2 hunks)src/value/path.rs(1 hunks)src/value/point.rs(1 hunks)src/value/vec32.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (26)
📓 Common learnings
Learnt from: gkorland
PR: FalkorDB/falkordb-rs#48
File: src/parser/mod.rs:82-86
Timestamp: 2024-10-13T19:43:33.128Z
Learning: When reviewing code in the FalkorDB Rust client, if `FalkorDBError` variants seem undefined, check if they are added in the same PR before flagging them as missing.
src/response/mod.rs (3)
Learnt from: barakb
PR: FalkorDB/falkordb-rs#48
File: src/response/index.rs:137-139
Timestamp: 2024-10-14T05:07:04.664Z
Learning: In `src/response/index.rs`, within the `FalkorIndex::parse` function, the `_properties` variable from the destructured array is intentionally ignored, as properties are not required for further processing.
Learnt from: gkorland
PR: FalkorDB/falkordb-rs#48
File: src/parser/mod.rs:82-86
Timestamp: 2024-10-13T19:43:33.128Z
Learning: When reviewing code in the FalkorDB Rust client, if `FalkorDBError` variants seem undefined, check if they are added in the same PR before flagging them as missing.
Learnt from: barakb
PR: FalkorDB/falkordb-rs#48
File: Cargo.toml:27-29
Timestamp: 2024-10-10T11:45:20.321Z
Learning: In the `falkordb` project, the `approx` crate is used in `src/client/blocking.rs` only within test code under `#[cfg(test)]`, so it's acceptable for it to be a development dependency.
src/client/builder.rs (2)
Learnt from: gkorland
PR: FalkorDB/falkordb-rs#48
File: src/parser/mod.rs:82-86
Timestamp: 2024-10-13T19:43:33.128Z
Learning: When reviewing code in the FalkorDB Rust client, if `FalkorDBError` variants seem undefined, check if they are added in the same PR before flagging them as missing.
Learnt from: barakb
PR: FalkorDB/falkordb-rs#48
File: Cargo.toml:27-29
Timestamp: 2024-10-10T11:45:20.321Z
Learning: In the `falkordb` project, the `approx` crate is used in `src/client/blocking.rs` only within test code under `#[cfg(test)]`, so it's acceptable for it to be a development dependency.
src/response/execution_plan.rs (4)
Learnt from: gkorland
PR: FalkorDB/falkordb-rs#48
File: src/parser/mod.rs:82-86
Timestamp: 2024-10-13T19:43:33.128Z
Learning: When reviewing code in the FalkorDB Rust client, if `FalkorDBError` variants seem undefined, check if they are added in the same PR before flagging them as missing.
Learnt from: barakb
PR: FalkorDB/falkordb-rs#48
File: Cargo.toml:27-29
Timestamp: 2024-10-10T11:45:20.321Z
Learning: In the `falkordb` project, the `approx` crate is used in `src/client/blocking.rs` only within test code under `#[cfg(test)]`, so it's acceptable for it to be a development dependency.
Learnt from: barakb
PR: FalkorDB/falkordb-rs#48
File: src/response/index.rs:137-139
Timestamp: 2024-10-14T05:07:04.664Z
Learning: In `src/response/index.rs`, within the `FalkorIndex::parse` function, the `_properties` variable from the destructured array is intentionally ignored, as properties are not required for further processing.
Learnt from: barakb
PR: FalkorDB/falkordb-rs#48
File: src/value/vec32.rs:0-0
Timestamp: 2024-10-14T04:50:57.723Z
Learning: Nested arrays are not legal inputs for Vec32 in FalkorDB.
src/value/point.rs (3)
Learnt from: gkorland
PR: FalkorDB/falkordb-rs#48
File: src/parser/mod.rs:82-86
Timestamp: 2024-10-13T19:43:33.128Z
Learning: When reviewing code in the FalkorDB Rust client, if `FalkorDBError` variants seem undefined, check if they are added in the same PR before flagging them as missing.
Learnt from: barakb
PR: FalkorDB/falkordb-rs#48
File: Cargo.toml:27-29
Timestamp: 2024-10-10T11:45:20.321Z
Learning: In the `falkordb` project, the `approx` crate is used in `src/client/blocking.rs` only within test code under `#[cfg(test)]`, so it's acceptable for it to be a development dependency.
Learnt from: barakb
PR: FalkorDB/falkordb-rs#48
File: src/response/index.rs:137-139
Timestamp: 2024-10-14T05:07:04.664Z
Learning: In `src/response/index.rs`, within the `FalkorIndex::parse` function, the `_properties` variable from the destructured array is intentionally ignored, as properties are not required for further processing.
src/response/constraint.rs (2)
Learnt from: barakb
PR: FalkorDB/falkordb-rs#48
File: src/response/index.rs:137-139
Timestamp: 2024-10-14T05:07:04.664Z
Learning: In `src/response/index.rs`, within the `FalkorIndex::parse` function, the `_properties` variable from the destructured array is intentionally ignored, as properties are not required for further processing.
Learnt from: gkorland
PR: FalkorDB/falkordb-rs#48
File: src/parser/mod.rs:82-86
Timestamp: 2024-10-13T19:43:33.128Z
Learning: When reviewing code in the FalkorDB Rust client, if `FalkorDBError` variants seem undefined, check if they are added in the same PR before flagging them as missing.
src/response/slowlog_entry.rs (3)
Learnt from: gkorland
PR: FalkorDB/falkordb-rs#48
File: src/parser/mod.rs:82-86
Timestamp: 2024-10-13T19:43:33.128Z
Learning: When reviewing code in the FalkorDB Rust client, if `FalkorDBError` variants seem undefined, check if they are added in the same PR before flagging them as missing.
Learnt from: barakb
PR: FalkorDB/falkordb-rs#48
File: Cargo.toml:27-29
Timestamp: 2024-10-10T11:45:20.321Z
Learning: In the `falkordb` project, the `approx` crate is used in `src/client/blocking.rs` only within test code under `#[cfg(test)]`, so it's acceptable for it to be a development dependency.
Learnt from: barakb
PR: FalkorDB/falkordb-rs#48
File: src/response/index.rs:137-139
Timestamp: 2024-10-14T05:07:04.664Z
Learning: In `src/response/index.rs`, within the `FalkorIndex::parse` function, the `_properties` variable from the destructured array is intentionally ignored, as properties are not required for further processing.
src/connection/blocking.rs (2)
Learnt from: gkorland
PR: FalkorDB/falkordb-rs#48
File: src/parser/mod.rs:82-86
Timestamp: 2024-10-13T19:43:33.128Z
Learning: When reviewing code in the FalkorDB Rust client, if `FalkorDBError` variants seem undefined, check if they are added in the same PR before flagging them as missing.
Learnt from: barakb
PR: FalkorDB/falkordb-rs#48
File: Cargo.toml:27-29
Timestamp: 2024-10-10T11:45:20.321Z
Learning: In the `falkordb` project, the `approx` crate is used in `src/client/blocking.rs` only within test code under `#[cfg(test)]`, so it's acceptable for it to be a development dependency.
src/lib.rs (3)
Learnt from: gkorland
PR: FalkorDB/falkordb-rs#48
File: src/parser/mod.rs:82-86
Timestamp: 2024-10-13T19:43:33.128Z
Learning: When reviewing code in the FalkorDB Rust client, if `FalkorDBError` variants seem undefined, check if they are added in the same PR before flagging them as missing.
Learnt from: barakb
PR: FalkorDB/falkordb-rs#48
File: src/response/index.rs:137-139
Timestamp: 2024-10-14T05:07:04.664Z
Learning: In `src/response/index.rs`, within the `FalkorIndex::parse` function, the `_properties` variable from the destructured array is intentionally ignored, as properties are not required for further processing.
Learnt from: barakb
PR: FalkorDB/falkordb-rs#48
File: Cargo.toml:27-29
Timestamp: 2024-10-10T11:45:20.321Z
Learning: In the `falkordb` project, the `approx` crate is used in `src/client/blocking.rs` only within test code under `#[cfg(test)]`, so it's acceptable for it to be a development dependency.
src/graph_schema/mod.rs (3)
Learnt from: gkorland
PR: FalkorDB/falkordb-rs#48
File: src/parser/mod.rs:82-86
Timestamp: 2024-10-13T19:43:33.128Z
Learning: When reviewing code in the FalkorDB Rust client, if `FalkorDBError` variants seem undefined, check if they are added in the same PR before flagging them as missing.
Learnt from: barakb
PR: FalkorDB/falkordb-rs#48
File: Cargo.toml:27-29
Timestamp: 2024-10-10T11:45:20.321Z
Learning: In the `falkordb` project, the `approx` crate is used in `src/client/blocking.rs` only within test code under `#[cfg(test)]`, so it's acceptable for it to be a development dependency.
Learnt from: barakb
PR: FalkorDB/falkordb-rs#48
File: src/response/index.rs:137-139
Timestamp: 2024-10-14T05:07:04.664Z
Learning: In `src/response/index.rs`, within the `FalkorIndex::parse` function, the `_properties` variable from the destructured array is intentionally ignored, as properties are not required for further processing.
src/value/graph_entities.rs (4)
Learnt from: barakb
PR: FalkorDB/falkordb-rs#48
File: src/response/index.rs:137-139
Timestamp: 2024-10-14T05:07:04.664Z
Learning: In `src/response/index.rs`, within the `FalkorIndex::parse` function, the `_properties` variable from the destructured array is intentionally ignored, as properties are not required for further processing.
Learnt from: gkorland
PR: FalkorDB/falkordb-rs#48
File: src/parser/mod.rs:82-86
Timestamp: 2024-10-13T19:43:33.128Z
Learning: When reviewing code in the FalkorDB Rust client, if `FalkorDBError` variants seem undefined, check if they are added in the same PR before flagging them as missing.
Learnt from: barakb
PR: FalkorDB/falkordb-rs#48
File: Cargo.toml:27-29
Timestamp: 2024-10-10T11:45:20.321Z
Learning: In the `falkordb` project, the `approx` crate is used in `src/client/blocking.rs` only within test code under `#[cfg(test)]`, so it's acceptable for it to be a development dependency.
Learnt from: barakb
PR: FalkorDB/falkordb-rs#48
File: src/value/vec32.rs:0-0
Timestamp: 2024-10-14T04:50:57.723Z
Learning: Nested arrays are not legal inputs for Vec32 in FalkorDB.
Cargo.toml (2)
Learnt from: barakb
PR: FalkorDB/falkordb-rs#48
File: Cargo.toml:27-29
Timestamp: 2024-10-10T11:45:20.321Z
Learning: In the `falkordb` project, the `approx` crate is used in `src/client/blocking.rs` only within test code under `#[cfg(test)]`, so it's acceptable for it to be a development dependency.
Learnt from: gkorland
PR: FalkorDB/falkordb-rs#48
File: src/parser/mod.rs:82-86
Timestamp: 2024-10-13T19:43:33.128Z
Learning: When reviewing code in the FalkorDB Rust client, if `FalkorDBError` variants seem undefined, check if they are added in the same PR before flagging them as missing.
src/graph/asynchronous.rs (3)
Learnt from: gkorland
PR: FalkorDB/falkordb-rs#48
File: src/parser/mod.rs:82-86
Timestamp: 2024-10-13T19:43:33.128Z
Learning: When reviewing code in the FalkorDB Rust client, if `FalkorDBError` variants seem undefined, check if they are added in the same PR before flagging them as missing.
Learnt from: barakb
PR: FalkorDB/falkordb-rs#48
File: Cargo.toml:27-29
Timestamp: 2024-10-10T11:45:20.321Z
Learning: In the `falkordb` project, the `approx` crate is used in `src/client/blocking.rs` only within test code under `#[cfg(test)]`, so it's acceptable for it to be a development dependency.
Learnt from: barakb
PR: FalkorDB/falkordb-rs#48
File: src/response/index.rs:137-139
Timestamp: 2024-10-14T05:07:04.664Z
Learning: In `src/response/index.rs`, within the `FalkorIndex::parse` function, the `_properties` variable from the destructured array is intentionally ignored, as properties are not required for further processing.
src/connection/asynchronous.rs (3)
Learnt from: gkorland
PR: FalkorDB/falkordb-rs#48
File: src/parser/mod.rs:82-86
Timestamp: 2024-10-13T19:43:33.128Z
Learning: When reviewing code in the FalkorDB Rust client, if `FalkorDBError` variants seem undefined, check if they are added in the same PR before flagging them as missing.
Learnt from: barakb
PR: FalkorDB/falkordb-rs#48
File: Cargo.toml:27-29
Timestamp: 2024-10-10T11:45:20.321Z
Learning: In the `falkordb` project, the `approx` crate is used in `src/client/blocking.rs` only within test code under `#[cfg(test)]`, so it's acceptable for it to be a development dependency.
Learnt from: barakb
PR: FalkorDB/falkordb-rs#48
File: src/response/index.rs:137-139
Timestamp: 2024-10-14T05:07:04.664Z
Learning: In `src/response/index.rs`, within the `FalkorIndex::parse` function, the `_properties` variable from the destructured array is intentionally ignored, as properties are not required for further processing.
src/client/asynchronous.rs (2)
Learnt from: gkorland
PR: FalkorDB/falkordb-rs#48
File: src/parser/mod.rs:82-86
Timestamp: 2024-10-13T19:43:33.128Z
Learning: When reviewing code in the FalkorDB Rust client, if `FalkorDBError` variants seem undefined, check if they are added in the same PR before flagging them as missing.
Learnt from: barakb
PR: FalkorDB/falkordb-rs#48
File: Cargo.toml:27-29
Timestamp: 2024-10-10T11:45:20.321Z
Learning: In the `falkordb` project, the `approx` crate is used in `src/client/blocking.rs` only within test code under `#[cfg(test)]`, so it's acceptable for it to be a development dependency.
src/graph/mod.rs (1)
Learnt from: gkorland
PR: FalkorDB/falkordb-rs#48
File: src/parser/mod.rs:82-86
Timestamp: 2024-10-13T19:43:33.128Z
Learning: When reviewing code in the FalkorDB Rust client, if `FalkorDBError` variants seem undefined, check if they are added in the same PR before flagging them as missing.
src/response/index.rs (3)
Learnt from: barakb
PR: FalkorDB/falkordb-rs#48
File: src/response/index.rs:137-139
Timestamp: 2024-10-14T05:07:04.664Z
Learning: In `src/response/index.rs`, within the `FalkorIndex::parse` function, the `_properties` variable from the destructured array is intentionally ignored, as properties are not required for further processing.
Learnt from: gkorland
PR: FalkorDB/falkordb-rs#48
File: src/parser/mod.rs:82-86
Timestamp: 2024-10-13T19:43:33.128Z
Learning: When reviewing code in the FalkorDB Rust client, if `FalkorDBError` variants seem undefined, check if they are added in the same PR before flagging them as missing.
Learnt from: barakb
PR: FalkorDB/falkordb-rs#48
File: Cargo.toml:27-29
Timestamp: 2024-10-10T11:45:20.321Z
Learning: In the `falkordb` project, the `approx` crate is used in `src/client/blocking.rs` only within test code under `#[cfg(test)]`, so it's acceptable for it to be a development dependency.
src/error/mod.rs (1)
Learnt from: gkorland
PR: FalkorDB/falkordb-rs#48
File: src/parser/mod.rs:82-86
Timestamp: 2024-10-13T19:43:33.128Z
Learning: When reviewing code in the FalkorDB Rust client, if `FalkorDBError` variants seem undefined, check if they are added in the same PR before flagging them as missing.
src/graph/query_builder.rs (3)
Learnt from: gkorland
PR: FalkorDB/falkordb-rs#48
File: src/parser/mod.rs:82-86
Timestamp: 2024-10-13T19:43:33.128Z
Learning: When reviewing code in the FalkorDB Rust client, if `FalkorDBError` variants seem undefined, check if they are added in the same PR before flagging them as missing.
Learnt from: barakb
PR: FalkorDB/falkordb-rs#48
File: Cargo.toml:27-29
Timestamp: 2024-10-10T11:45:20.321Z
Learning: In the `falkordb` project, the `approx` crate is used in `src/client/blocking.rs` only within test code under `#[cfg(test)]`, so it's acceptable for it to be a development dependency.
Learnt from: barakb
PR: FalkorDB/falkordb-rs#48
File: src/response/index.rs:137-139
Timestamp: 2024-10-14T05:07:04.664Z
Learning: In `src/response/index.rs`, within the `FalkorIndex::parse` function, the `_properties` variable from the destructured array is intentionally ignored, as properties are not required for further processing.
src/value/vec32.rs (4)
Learnt from: gkorland
PR: FalkorDB/falkordb-rs#48
File: src/parser/mod.rs:82-86
Timestamp: 2024-10-13T19:43:33.128Z
Learning: When reviewing code in the FalkorDB Rust client, if `FalkorDBError` variants seem undefined, check if they are added in the same PR before flagging them as missing.
Learnt from: barakb
PR: FalkorDB/falkordb-rs#48
File: src/value/vec32.rs:0-0
Timestamp: 2024-10-14T04:50:57.723Z
Learning: Nested arrays are not legal inputs for Vec32 in FalkorDB.
Learnt from: barakb
PR: FalkorDB/falkordb-rs#48
File: src/response/index.rs:137-139
Timestamp: 2024-10-14T05:07:04.664Z
Learning: In `src/response/index.rs`, within the `FalkorIndex::parse` function, the `_properties` variable from the destructured array is intentionally ignored, as properties are not required for further processing.
Learnt from: barakb
PR: FalkorDB/falkordb-rs#48
File: Cargo.toml:27-29
Timestamp: 2024-10-10T11:45:20.321Z
Learning: In the `falkordb` project, the `approx` crate is used in `src/client/blocking.rs` only within test code under `#[cfg(test)]`, so it's acceptable for it to be a development dependency.
src/response/lazy_result_set.rs (3)
Learnt from: gkorland
PR: FalkorDB/falkordb-rs#48
File: src/parser/mod.rs:82-86
Timestamp: 2024-10-13T19:43:33.128Z
Learning: When reviewing code in the FalkorDB Rust client, if `FalkorDBError` variants seem undefined, check if they are added in the same PR before flagging them as missing.
Learnt from: barakb
PR: FalkorDB/falkordb-rs#48
File: src/response/index.rs:137-139
Timestamp: 2024-10-14T05:07:04.664Z
Learning: In `src/response/index.rs`, within the `FalkorIndex::parse` function, the `_properties` variable from the destructured array is intentionally ignored, as properties are not required for further processing.
Learnt from: barakb
PR: FalkorDB/falkordb-rs#48
File: Cargo.toml:27-29
Timestamp: 2024-10-10T11:45:20.321Z
Learning: In the `falkordb` project, the `approx` crate is used in `src/client/blocking.rs` only within test code under `#[cfg(test)]`, so it's acceptable for it to be a development dependency.
src/client/mod.rs (3)
Learnt from: gkorland
PR: FalkorDB/falkordb-rs#48
File: src/parser/mod.rs:82-86
Timestamp: 2024-10-13T19:43:33.128Z
Learning: When reviewing code in the FalkorDB Rust client, if `FalkorDBError` variants seem undefined, check if they are added in the same PR before flagging them as missing.
Learnt from: barakb
PR: FalkorDB/falkordb-rs#48
File: Cargo.toml:27-29
Timestamp: 2024-10-10T11:45:20.321Z
Learning: In the `falkordb` project, the `approx` crate is used in `src/client/blocking.rs` only within test code under `#[cfg(test)]`, so it's acceptable for it to be a development dependency.
Learnt from: barakb
PR: FalkorDB/falkordb-rs#48
File: src/response/index.rs:137-139
Timestamp: 2024-10-14T05:07:04.664Z
Learning: In `src/response/index.rs`, within the `FalkorIndex::parse` function, the `_properties` variable from the destructured array is intentionally ignored, as properties are not required for further processing.
src/client/blocking.rs (3)
Learnt from: gkorland
PR: FalkorDB/falkordb-rs#48
File: src/parser/mod.rs:82-86
Timestamp: 2024-10-13T19:43:33.128Z
Learning: When reviewing code in the FalkorDB Rust client, if `FalkorDBError` variants seem undefined, check if they are added in the same PR before flagging them as missing.
Learnt from: barakb
PR: FalkorDB/falkordb-rs#48
File: Cargo.toml:27-29
Timestamp: 2024-10-10T11:45:20.321Z
Learning: In the `falkordb` project, the `approx` crate is used in `src/client/blocking.rs` only within test code under `#[cfg(test)]`, so it's acceptable for it to be a development dependency.
Learnt from: barakb
PR: FalkorDB/falkordb-rs#48
File: src/response/index.rs:137-139
Timestamp: 2024-10-14T05:07:04.664Z
Learning: In `src/response/index.rs`, within the `FalkorIndex::parse` function, the `_properties` variable from the destructured array is intentionally ignored, as properties are not required for further processing.
src/value/mod.rs (1)
Learnt from: gkorland
PR: FalkorDB/falkordb-rs#48
File: src/parser/mod.rs:82-86
Timestamp: 2024-10-13T19:43:33.128Z
Learning: When reviewing code in the FalkorDB Rust client, if `FalkorDBError` variants seem undefined, check if they are added in the same PR before flagging them as missing.
src/graph/blocking.rs (3)
Learnt from: barakb
PR: FalkorDB/falkordb-rs#48
File: Cargo.toml:27-29
Timestamp: 2024-10-10T11:45:20.321Z
Learning: In the `falkordb` project, the `approx` crate is used in `src/client/blocking.rs` only within test code under `#[cfg(test)]`, so it's acceptable for it to be a development dependency.
Learnt from: gkorland
PR: FalkorDB/falkordb-rs#48
File: src/parser/mod.rs:82-86
Timestamp: 2024-10-13T19:43:33.128Z
Learning: When reviewing code in the FalkorDB Rust client, if `FalkorDBError` variants seem undefined, check if they are added in the same PR before flagging them as missing.
Learnt from: barakb
PR: FalkorDB/falkordb-rs#48
File: src/response/index.rs:137-139
Timestamp: 2024-10-14T05:07:04.664Z
Learning: In `src/response/index.rs`, within the `FalkorIndex::parse` function, the `_properties` variable from the destructured array is intentionally ignored, as properties are not required for further processing.
src/parser/mod.rs (2)
Learnt from: gkorland
PR: FalkorDB/falkordb-rs#48
File: src/parser/mod.rs:82-86
Timestamp: 2024-10-13T19:43:33.128Z
Learning: When reviewing code in the FalkorDB Rust client, if `FalkorDBError` variants seem undefined, check if they are added in the same PR before flagging them as missing.
Learnt from: barakb
PR: FalkorDB/falkordb-rs#48
File: Cargo.toml:27-29
Timestamp: 2024-10-10T11:45:20.321Z
Learning: In the `falkordb` project, the `approx` crate is used in `src/client/blocking.rs` only within test code under `#[cfg(test)]`, so it's acceptable for it to be a development dependency.
🧬 Code Graph Analysis (12)
src/response/mod.rs (1)
src/parser/mod.rs (2)
parse_header(230-264)redis_value_as_untyped_string_vec(221-224)
src/value/path.rs (1)
src/parser/mod.rs (1)
redis_value_as_vec(95-100)
src/response/execution_plan.rs (1)
src/parser/mod.rs (2)
redis_value_as_string(59-68)redis_value_as_vec(95-100)
src/value/point.rs (1)
src/parser/mod.rs (2)
redis_value_as_double(85-88)redis_value_as_vec(95-100)
src/response/constraint.rs (1)
src/parser/mod.rs (4)
parse_falkor_enum(161-176)redis_value_as_typed_string(186-194)redis_value_as_typed_string_vec(200-215)redis_value_as_vec(95-100)
src/response/slowlog_entry.rs (1)
src/parser/mod.rs (3)
redis_value_as_double(85-88)redis_value_as_string(59-68)redis_value_as_vec(95-100)
src/graph_schema/mod.rs (3)
src/parser/mod.rs (4)
parse_type(324-376)redis_value_as_int(70-75)redis_value_as_string(59-68)redis_value_as_vec(95-100)src/client/blocking.rs (1)
create_empty_inner_sync_client(241-250)src/lib.rs (1)
create_test_client(87-91)
src/graph/asynchronous.rs (1)
src/lib.rs (2)
create_async_test_client(94-99)open_empty_async_test_graph(110-116)
src/client/asynchronous.rs (2)
src/parser/mod.rs (2)
parse_config_hashmap(122-155)redis_value_as_untyped_string_vec(221-224)src/lib.rs (1)
create_async_test_client(94-99)
src/response/index.rs (2)
src/parser/mod.rs (6)
parse_type(324-376)parse_falkor_enum(161-176)parse_raw_redis_value(269-275)redis_value_as_string(59-68)redis_value_as_vec(95-100)type_val_from_value(281-298)src/value/mod.rs (1)
as_vec(90-95)
src/graph/query_builder.rs (1)
src/parser/mod.rs (1)
redis_value_as_vec(95-100)
src/value/vec32.rs (1)
src/parser/mod.rs (2)
redis_value_as_float(90-93)redis_value_as_vec(95-100)
🔇 Additional comments (37)
.gitignore (1)
9-9: 👍 Good call adding.DS_StoreIgnoring macOS metadata files keeps the repo clean. No further action needed.
src/response/mod.rs (1)
6-9: Import re-ordering is finePurely cosmetic; keeps parser helpers grouped with other crate imports. No functional impact.
src/value/vec32.rs (1)
6-10: No objection to the import shuffleChange is stylistic only. Existing parsing logic and tests remain unaffected.
Cargo.toml (1)
19-19: Chrono pin looks good
chrono 0.4.41is the latest 0.4.x as of today; nothing else to flag.src/value/graph_entities.rs (1)
84-90: Destructuring expanded – clearer, no behavioural changeThe multiline pattern is easier to read and matches style adopted elsewhere in the PR.
src/response/constraint.rs (1)
62-74: Readability improvement acknowledgedExpanded destructuring and explicit closure body improve clarity. Logic and error paths remain identical.
src/response/execution_plan.rs (1)
6-9: Import re-ordering looks finePurely stylistic change; no functional impact.
src/response/slowlog_entry.rs (1)
6-9: Import re-ordering appears safeJust a cosmetic shuffle—no objections.
src/client/mod.rs (1)
6-9: Import block reordered – OKRe-grouping
FalkorDBError/FalkorResultbefore the other modules improves readability and mirrors the pattern in the rest of the PR.src/client/builder.rs (1)
6-9: Import block re-ordering looks goodMaintains consistency with other modules.
src/response/lazy_result_set.rs (1)
7-7: Import order change looks goodPurely cosmetic shuffle – no functional impact or re-exports affected. ✅
src/lib.rs (1)
34-35: Re-export reorder is non-breaking
QueryResultandFalkorValueare still publicly exposed; only their position in the list changed. No downstream code should rely on ordering, so this is safe.Also applies to: 42-43
src/value/point.rs (1)
6-9: Minor import rearrangement acknowledgedNo behavioural change; the file continues to compile and tests remain valid.
src/graph_schema/mod.rs (2)
7-12: Import block reorder is fineGrouping
FalkorDBError,FalkorResult, andFalkorValuefirst improves readability without altering behaviour.
279-281: Test-only import shuffle OKThe altered order in the test module doesn’t affect compilation or test execution.
src/connection/asynchronous.rs (1)
7-9: Async connection import tweak approvedCosmetic only; functionality untouched.
src/response/index.rs (1)
121-137: Confirm element ordering with the Redis protocolThe destructuring now assumes the array is laid out as
[label, fields, field_types, options, language, stopwords, entity_type, status, info].
If the server ever changes the order ofentity_typeandstatus(or vice-versa), parsing will silently swap the two and yield bogus results.Please double-check the upstream response format or add an integration test that would fail loudly on a mismatch.
src/connection/blocking.rs (1)
7-15: Import shuffle only – looks goodOnly ordering of imports changed; no functional impact detected.
src/client/asynchronous.rs (1)
6-19: Imports reordered for clarityPurely cosmetic re-ordering; implementation untouched.
Also applies to: 275-276
src/graph/blocking.rs (1)
9-12: Imports realigned – no further commentsNo behavioural change; compilation path remains identical.
Also applies to: 396-396
src/graph/asynchronous.rs (1)
7-13: Imports only – change is benignJust an ordering tweak; the runtime behaviour is unaffected.
Also applies to: 406-408
src/error/mod.rs (2)
24-26: LGTM! Formatting improvements enhance readability.The multi-line string formatting for error messages improves code readability while maintaining the same error text.
Also applies to: 41-43, 120-122, 128-130
135-137: Good addition of temporal error handling.The new
ParseTemporalErrorvariant properly supports the temporal types being added. The error message format is consistent with existing patterns and provides clear feedback for invalid temporal values.src/graph/mod.rs (1)
28-28: Excellent string formatting modernization.The migration from positional to named arguments in format strings significantly improves readability and maintainability. These changes align with modern Rust best practices.
Also applies to: 33-34, 51-51, 54-54, 65-65, 70-71, 81-81
src/graph/query_builder.rs (3)
9-10: Good import organization.The import reordering improves code structure and readability.
86-86: Documentation improvement.Simplifying the documentation comment by removing redundancy makes it more concise while maintaining clarity.
267-267: Consistent string formatting modernization.The adoption of named arguments in format strings matches the improvements made across other files and enhances code readability.
Also applies to: 270-270
src/value/mod.rs (2)
48-53: Well-designed temporal type support.The addition of
DateTime,Date, andTimevariants using chrono types is excellent:
- Uses standard Rust temporal types from the chrono crate
DateTime<Utc>is appropriate for database timestampsNaiveDateandNaiveTimeare suitable for date/time values without timezone context- Integrates seamlessly with the existing
FalkorValueenum
163-192: Consistent accessor method implementation.The new accessor methods follow the established pattern perfectly:
- Consistent naming convention (
as_date_time,as_date,as_time)- Proper documentation with return types
- Same pattern matching approach as other accessors
- Returns
Option<&T>for safe accesssrc/client/blocking.rs (4)
7-7: Good import organization improvements.The import reordering and addition of chrono traits for testing enhance code organization and support the new temporal functionality.
Also applies to: 15-15, 258-258, 261-261
141-141: Documentation formatting fixes.The bullet point formatting corrections in the documentation improve readability.
Also applies to: 164-164
325-325: String interpolation modernization.Consistent with other files, this change improves code readability by using named arguments in format strings.
366-419: Excellent test coverage for temporal features.The three new tests provide comprehensive coverage of the temporal functionality:
test_get_time(): Validateslocaltimeparsing and hour extractiontest_get_date(): Validatesdateparsing and year extractiontest_get_date_time(): Validateslocaldatetimeparsing and year extractionThe tests properly:
- Use realistic database queries with temporal functions
- Assert on specific temporal components using chrono traits
- Handle error cases with descriptive panic messages
- Follow the established testing pattern in the codebase
src/parser/mod.rs (4)
6-6: LGTM: Clean import addition for temporal support.The chrono imports are appropriately scoped and follow the existing import organization pattern.
29-31: LGTM: Consistent enum variant additions.The new temporal type markers follow the existing sequential numbering pattern and maintain consistency with the parser architecture.
51-53: LGTM: Proper TryFrom implementation extension.The new match arms correctly map the integer values to their corresponding parser type markers, maintaining consistency with the existing implementation.
389-391: Minor: Import reordering is cosmetic.The import reordering doesn't affect functionality and appears to improve readability by grouping related imports together.
| name = "falkordb" | ||
| version = "0.1.11" | ||
| edition = "2021" | ||
| edition = "2024" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Bumping to edition 2024 is risky – confirm MSRV & CI images
Edition 2024 has not hit stable as of Rust 1.79. Unless your CI/build matrix already compiles with a nightly or future stable, this will break every downstream consumer pinned to stable 1.78 or older.
-edition = "2024"
+edition = "2021" # keep until 2024 edition reaches stable toolchainsAlternatively, document the new minimum Rust version and update CI to use the corresponding toolchain.
🏁 Script executed:
#!/bin/bash
set -e
echo "=== Checking for rust-toolchain files ==="
fd -t f rust-toolchain* || echo "No rust-toolchain files found."
echo -e "\n=== Contents of rust-toolchain files (if any) ==="
fd -t f rust-toolchain* -x cat {} || true
echo -e "\n=== Listing GitHub Actions workflow files ==="
find .github/workflows -type f || echo "No workflows directory."
echo -e "\n=== Searching workflows for Rust toolchain versions ==="
grep -R "actions-rs/toolchain" -n .github/workflows || echo "No uses of actions-rs/toolchain."
grep -R "rust-version" -n .github/workflows || echo "No rust-version fields."
grep -R "stable" -n .github/workflows || true
grep -R "nightly" -n .github/workflows || trueLength of output: 993
Confirm MSRV & CI toolchain before bumping to Edition 2024
Edition 2024 isn’t stabilized in Rust 1.79. Every downstream crate building on stable ≤1.78 will break unless we:
- Document and enforce a new minimum Rust version (e.g. ≥1.80)
- Update CI to install/override to nightly or the required stable toolchain
Suggested fixes:
- Revert to 2021 edition until 2024 lands in stable toolchains, or
- Add a
rust-toolchainfile or CI step (e.g.actions-rs/toolchain) to pin ≥1.80 or nightly, and update README/MSRV documentation
- edition = "2024"
+ edition = "2021" # until Edition 2024 is supported on stable toolchains📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| edition = "2024" | |
| edition = "2021" # until Edition 2024 is supported on stable toolchains |
🤖 Prompt for AI Agents
In Cargo.toml at line 4, the edition is set to 2024, which is not yet stabilized
in Rust 1.79 and may break builds on stable versions ≤1.78. To fix this, either
revert the edition back to 2021 until Rust 2024 is stable, or add a
rust-toolchain file or update the CI configuration to pin the Rust toolchain to
version 1.80 or nightly. Also, update the README and MSRV documentation to
reflect the new minimum Rust version requirement.
| ParserTypeMarker::DateTime => FalkorValue::DateTime( | ||
| DateTime::<Utc>::from_timestamp(redis_value_as_int(val)?, 0).ok_or( | ||
| FalkorDBError::ParseTemporalError( | ||
| "Could not parse date time from timestamp".to_string(), | ||
| ), | ||
| )?, | ||
| ), | ||
| ParserTypeMarker::Date => FalkorValue::Date( | ||
| DateTime::<Utc>::from_timestamp(redis_value_as_int(val)?, 0) | ||
| .map(|dt| dt.date_naive()) | ||
| .ok_or(FalkorDBError::ParseTemporalError( | ||
| "Could not parse date from timestamp".to_string(), | ||
| ))?, | ||
| ), | ||
| ParserTypeMarker::Time => FalkorValue::Time( | ||
| DateTime::<Utc>::from_timestamp(redis_value_as_int(val)?, 0) | ||
| .map(|dt| dt.time()) | ||
| .ok_or(FalkorDBError::ParseTemporalError( | ||
| "Could not parse time from timestamp".to_string(), | ||
| ))?, | ||
| ), |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify timestamp precision and consider edge cases.
The temporal parsing implementation is well-structured and follows consistent patterns. However, please verify:
-
Timestamp precision: The implementation assumes timestamps are in seconds (nanoseconds hardcoded to 0). Confirm this matches the expected data format from FalkorDB.
-
Timestamp range: Consider adding validation for timestamp bounds to provide more specific error messages for out-of-range values.
Run the following script to verify the ParseTemporalError variant exists and check for any temporal-related tests:
🏁 Script executed:
#!/bin/bash
# Description: Verify ParseTemporalError exists and check for temporal tests
# Check if ParseTemporalError variant exists
echo "=== Checking for ParseTemporalError in error module ==="
fd -e rs -x grep -l "ParseTemporalError" {} \;
# Check for temporal-related test implementations
echo "=== Checking for temporal type tests ==="
rg -A 10 -B 5 "DateTime.*test|Date.*test|Time.*test|test.*DateTime|test.*Date|test.*Time"
# Check FalkorValue enum for temporal variants
echo "=== Checking FalkorValue enum for temporal variants ==="
ast-grep --pattern 'enum FalkorValue {
$$$
}'Length of output: 563
Add precision checks, range validation, and tests for temporal parsing
We’ve confirmed that the ParseTemporalError variant is defined in src/error/mod.rs, but there are currently no unit tests covering DateTime/Date/Time parsing. Please address the following:
- Verify and document that Redis timestamps are expressed in seconds (nanoseconds hardcoded to 0).
- Add explicit bounds checks on the integer timestamp (e.g., before calling
from_timestamp) to return a clear error when values are out of the supported range. - Introduce dedicated tests (e.g., in
tests/parser_temporal.rsor similar) to cover:- Successful parsing of valid second‐precision timestamps for each of DateTime, Date, and Time.
- Failure cases, including out‐of‐range timestamps and non-integer inputs, to exercise the
ParseTemporalErrorpaths.
🤖 Prompt for AI Agents
In src/parser/mod.rs around lines 352 to 372, add explicit bounds checks on the
integer timestamp before calling from_timestamp to ensure it is within the valid
range for seconds, returning a ParseTemporalError if out of range. Confirm and
document that Redis timestamps are in seconds with nanoseconds set to zero.
Then, create dedicated unit tests in a new file like tests/parser_temporal.rs to
cover successful parsing of valid second-precision timestamps for DateTime,
Date, and Time, as well as failure cases including out-of-range timestamps and
non-integer inputs to trigger ParseTemporalError.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
Cargo.toml (1)
4-4: Edition 2024: confirm MSRV/toolchain before mergingIf CI and README don’t already declare a Rust toolchain that supports Edition 2024, add a
rust-versionin[package]and pin CI/toolchain accordingly. Otherwise downstream users on older stable will break.Run to verify/pin MSRV and CI toolchain:
#!/bin/bash set -euo pipefail echo "=== Check rust-version in Cargo.toml ===" rg -n '^rust-version\s*=' Cargo.toml || echo "Missing [package].rust-version" echo -e "\n=== rust-toolchain files ===" fd -t f "rust-toolchain*" -x cat {} echo -e "\n=== Workflows using fixed toolchains ===" fd -t f .github/workflows -x rg -n 'toolchain|rust-version|rustup|stable|beta|nightly' {}
🧹 Nitpick comments (2)
src/client/blocking.rs (1)
423-494: Duration construction test: good coverage; consider batching to reduce round-trips
You could return multiple durations in a single query to speed up CI, but current form is acceptable.src/value/mod.rs (1)
301-343: TryFrom -> chrono types: LGTM
Optional: addTryFrom<&FalkorValue>to avoid moves at call sites.+impl<'a> TryFrom<&'a FalkorValue> for chrono::Duration { + type Error = FalkorDBError; + fn try_from(value: &'a FalkorValue) -> Result<Self, Self::Error> { + match value { + FalkorValue::Duration(d) => Ok(*d), + _ => Err(FalkorDBError::ParseTemporalError("Not a Duration value".into())), + } + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
Cargo.toml(2 hunks)src/client/blocking.rs(7 hunks)src/parser/mod.rs(7 hunks)src/value/mod.rs(5 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: gkorland
PR: FalkorDB/falkordb-rs#48
File: src/parser/mod.rs:82-86
Timestamp: 2024-10-13T19:43:33.128Z
Learning: When reviewing code in the FalkorDB Rust client, if `FalkorDBError` variants seem undefined, check if they are added in the same PR before flagging them as missing.
📚 Learning: 2024-10-13T19:43:33.128Z
Learnt from: gkorland
PR: FalkorDB/falkordb-rs#48
File: src/parser/mod.rs:82-86
Timestamp: 2024-10-13T19:43:33.128Z
Learning: When reviewing code in the FalkorDB Rust client, if `FalkorDBError` variants seem undefined, check if they are added in the same PR before flagging them as missing.
Applied to files:
src/parser/mod.rssrc/client/blocking.rs
📚 Learning: 2024-10-10T11:45:20.321Z
Learnt from: barakb
PR: FalkorDB/falkordb-rs#48
File: Cargo.toml:27-29
Timestamp: 2024-10-10T11:45:20.321Z
Learning: In the `falkordb` project, the `approx` crate is used in `src/client/blocking.rs` only within test code under `#[cfg(test)]`, so it's acceptable for it to be a development dependency.
Applied to files:
src/parser/mod.rsCargo.tomlsrc/client/blocking.rs
🧬 Code graph analysis (3)
src/parser/mod.rs (2)
src/graph_schema/mod.rs (2)
new(87-99)open_readonly_graph_with_modified_schema(284-303)src/client/blocking.rs (1)
create_empty_inner_sync_client(241-250)
src/client/blocking.rs (2)
src/lib.rs (1)
create_test_client(87-91)src/value/mod.rs (8)
as_time(213-218)as_date(203-208)as_date_time(193-198)from(82-84)from(88-90)from(94-96)from(100-102)from(106-108)
src/value/mod.rs (1)
src/parser/mod.rs (1)
try_from(38-58)
🪛 GitHub Actions: Mandatory Pull Request Checks
src/parser/mod.rs
[error] 181-184: Rustfmt failed due to trailing whitespace in src/parser/mod.rs (lines 181-184).
🪛 GitHub Actions: Code Coverage
src/parser/mod.rs
[error] 922-922: Test 'parser::tests::test_parse_duration_date_only' failed: assertion 'res.is_ok()' failed.
[error] 880-880: Test 'parser::tests::test_parse_duration_simple' failed: assertion 'res.is_ok()' failed.
[warning] 109-109: Function 'parse_duration_from_string' is never used.
🔇 Additional comments (18)
Cargo.toml (1)
19-24: Approve dependency version bumps: Resolved versions in Cargo.lock match the intended bumps;thiserror::Errorderive and Redis sentinel features are used as expected; no breaking changes or MSRV shifts detected.src/client/blocking.rs (8)
7-16: Import and std::sync reorderings: LGTM
141-141: Doc tweak: LGTM
165-165: Doc tweak: LGTM
259-262: Test imports for chrono traits: LGTM
345-345: Iteration refactor in test_read_vec32: LGTM
366-384: Temporal test (localtime): LGTM
386-402: Temporal test (date): LGTM
404-421: Temporal test (localdatetime): LGTMsrc/value/mod.rs (4)
48-56: New temporal FalkorValue variants: LGTM
87-110: From impls for chrono types: LGTM
189-229: Accessors for chrono types: LGTM
470-535: Temporal tests in value module: LGTMsrc/parser/mod.rs (5)
6-11: Imports and Vec32 path: LGTM
29-33: ParserTypeMarker additions: LGTM
52-56: TryFrom mapping for new markers: LGTM
871-965: Manual verification required: CI couldn’t runcargo fmtor the duration tests due to environment limits; please runcargo fmt -- --checkand verifyparser::tests::test_parse_duration_*pass now thatparse_typesupports string durations.
437-457: Confirm timestamp units for temporal parsing (src/parser/mod.rs:437–457)
Code usesDateTime::<Utc>::from_timestamp(..., 0), which expects seconds since the UNIX epoch. Verify that the Redis server encodes timestamps in seconds (not milliseconds); if it returns milliseconds, divide by 1000 before parsing.
src/parser/mod.rs
Outdated
| ParserTypeMarker::Duration => FalkorValue::Duration( | ||
| chrono::Duration::seconds(redis_value_as_int(val)?) | ||
| ), | ||
| }; |
There was a problem hiding this comment.
Duration parsing mismatch causes test failures — accept string or int
parse_type currently expects an integer but tests pass ISO-like strings (e.g., "P1Y2M..."). Use parse_duration_from_string for strings, keep integer seconds support. This also resolves the “function never used” warning.
Apply:
- ParserTypeMarker::Duration => FalkorValue::Duration(
- chrono::Duration::seconds(redis_value_as_int(val)?)
- ),
+ ParserTypeMarker::Duration => {
+ let duration = match val {
+ redis::Value::Int(secs) => chrono::Duration::seconds(secs),
+ redis::Value::BulkString(bytes) => {
+ let s = std::str::from_utf8(&bytes).map_err(|_| FalkorDBError::ParsingString)?;
+ parse_duration_from_string(s)?
+ }
+ redis::Value::SimpleString(s) => parse_duration_from_string(&s)?,
+ redis::Value::VerbatimString { text, .. } => parse_duration_from_string(&text)?,
+ _ => {
+ return Err(FalkorDBError::ParseTemporalError(
+ "Expected duration as integer seconds or ISO 8601-like string".to_string(),
+ ))
+ }
+ };
+ FalkorValue::Duration(duration)
+ },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ParserTypeMarker::Duration => FalkorValue::Duration( | |
| chrono::Duration::seconds(redis_value_as_int(val)?) | |
| ), | |
| }; | |
| ParserTypeMarker::Duration => { | |
| let duration = match val { | |
| redis::Value::Int(secs) => chrono::Duration::seconds(secs), | |
| redis::Value::BulkString(bytes) => { | |
| let s = std::str::from_utf8(&bytes) | |
| .map_err(|_| FalkorDBError::ParsingString)?; | |
| parse_duration_from_string(s)? | |
| } | |
| redis::Value::SimpleString(s) => parse_duration_from_string(&s)?, | |
| redis::Value::VerbatimString { text, .. } => parse_duration_from_string(&text)?, | |
| _ => { | |
| return Err(FalkorDBError::ParseTemporalError( | |
| "Expected duration as integer seconds or ISO 8601-like string".to_string(), | |
| )) | |
| } | |
| }; | |
| FalkorValue::Duration(duration) | |
| }, |
🤖 Prompt for AI Agents
In src/parser/mod.rs around lines 458 to 461, the Duration branch currently
assumes the redis value is an integer seconds; update it to accept either an
integer or an ISO-like duration string: if the redis value is a string, call
parse_duration_from_string(on the string) and construct FalkorValue::Duration
from that result; otherwise if it is an integer, keep using
chrono::Duration::seconds(redis_value_as_int(val)?). Ensure proper error
propagation and remove the “function never used” warning by actually invoking
parse_duration_from_string in this branch.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/parser/mod.rs (2)
464-466: Duration parse bug: strings not supported though tests pass strings.parse_type expects an integer, but tests construct SimpleString (e.g., "P1Y2M…"). Use parse_duration_from_string and also keep integer-seconds support.
Apply:
- ParserTypeMarker::Duration => { - FalkorValue::Duration(chrono::Duration::seconds(redis_value_as_int(val)?)) - } + ParserTypeMarker::Duration => { + let duration = match val { + redis::Value::Int(secs) => chrono::Duration::seconds(secs), + redis::Value::BulkString(bytes) => { + let s = std::str::from_utf8(&bytes) + .map_err(|_| FalkorDBError::ParsingString)?; + parse_duration_from_string(s)? + } + redis::Value::SimpleString(s) => parse_duration_from_string(&s)?, + redis::Value::VerbatimString { text, .. } => parse_duration_from_string(&text)?, + other => { + return Err(FalkorDBError::ParseTemporalError( + format!("Expected duration as integer seconds or ISO-like string, got {:?}", other) + )) + } + }; + FalkorValue::Duration(duration) + }
104-191: Harden duration string parser: reject decimals, handle overflow, optional 'W', and trailing number.Currently accepts '.' but parses as i64; risks overflow; leaves trailing digits unvalidated. Use checked i128 math; optionally support 'W' weeks; error on leftover number.
Apply:
@@ - let mut current_number = String::new(); + let mut current_number = String::new(); let mut in_time_part = false; @@ - '0'..='9' | '.' => { + '0'..='9' => { current_number.push(ch); } + 'W' if !in_time_part => { + let weeks: i64 = current_number.parse().map_err(|_| { + FalkorDBError::ParseTemporalError("Invalid week value in duration".to_string()) + })?; + days = days.checked_add(weeks.checked_mul(7).ok_or_else(|| FalkorDBError::ParseTemporalError("Duration value out of range".into()))?) + .ok_or_else(|| FalkorDBError::ParseTemporalError("Duration value out of range".into()))?; + current_number.clear(); + } _ => { return Err(FalkorDBError::ParseTemporalError(format!( "Invalid character '{}' in duration string", ch ))); } } } - - // Convert to total seconds (approximate for years/months) - let total_seconds = seconds + - minutes * 60 + - hours * 3600 + - days * 86400 + - months * 30 * 86400 + // Approximate: 30 days per month - years * 365 * 86400; // Approximate: 365 days per year - - chrono::Duration::try_seconds(total_seconds).ok_or(FalkorDBError::ParseTemporalError( - "Duration value out of range".to_string(), - )) + // No dangling number without a unit + if !current_number.is_empty() { + return Err(FalkorDBError::ParseTemporalError( + "Dangling number without a unit in duration string".to_string(), + )); + } + + // Convert to total seconds (approximate for years/months) using checked i128 math + let to_i128 = |v: i64| v as i128; + let sec = to_i128(seconds); + let min = to_i128(minutes).checked_mul(60).ok_or_else(|| FalkorDBError::ParseTemporalError("Duration value out of range".into()))?; + let hr = to_i128(hours).checked_mul(3_600).ok_or_else(|| FalkorDBError::ParseTemporalError("Duration value out of range".into()))?; + let day = to_i128(days).checked_mul(86_400).ok_or_else(|| FalkorDBError::ParseTemporalError("Duration value out of range".into()))?; + let mon = to_i128(months).checked_mul(30).and_then(|d| d.checked_mul(86_400)).ok_or_else(|| FalkorDBError::ParseTemporalError("Duration value out of range".into()))?; + let yr = to_i128(years).checked_mul(365).and_then(|d| d.checked_mul(86_400)).ok_or_else(|| FalkorDBError::ParseTemporalError("Duration value out of range".into()))?; + let total = sec.checked_add(min).and_then(|t| t.checked_add(hr)).and_then(|t| t.checked_add(day)).and_then(|t| t.checked_add(mon)).and_then(|t| t.checked_add(yr)) + .ok_or_else(|| FalkorDBError::ParseTemporalError("Duration value out of range".into()))?; + + chrono::Duration::try_seconds(total as i64) + .ok_or(FalkorDBError::ParseTemporalError("Duration value out of range".to_string()))Additionally, remove any trailing whitespace and run cargo fmt.
🧹 Nitpick comments (3)
src/client/blocking.rs (2)
139-142: Doc nit: fix parameter name casing in docs (config_Key→config_key).Keeps docs consistent with the actual parameter.
Apply:
-/// * `config_Key`: A [`String`] representation of a configuration's key. +/// * `config_key`: A [`String`] representation of a configuration's key.Also applies to: 162-166
423-509: Solid duration coverage; minor suggestions.
- Consider adding a “weeks + days” mixed case for completeness.
- Optionally clean up by deleting the temporary graph at the end (if it is materialized in your env).
src/value/mod.rs (1)
301-351: Consider also supporting borrowed conversions (TryFrom<&FalkorValue>).Avoids moves when callers only need refs. Backwards compatible.
Example:
+impl TryFrom<&FalkorValue> for chrono::Duration { + type Error = FalkorDBError; + fn try_from(value: &FalkorValue) -> Result<Self, Self::Error> { + match value { + FalkorValue::Duration(d) => Ok(*d), + _ => Err(FalkorDBError::ParseTemporalError("Not a Duration value".into())), + } + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/client/blocking.rs(7 hunks)src/parser/mod.rs(7 hunks)src/value/mod.rs(5 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: gkorland
PR: FalkorDB/falkordb-rs#48
File: src/parser/mod.rs:82-86
Timestamp: 2024-10-13T19:43:33.128Z
Learning: When reviewing code in the FalkorDB Rust client, if `FalkorDBError` variants seem undefined, check if they are added in the same PR before flagging them as missing.
📚 Learning: 2024-10-13T19:43:33.128Z
Learnt from: gkorland
PR: FalkorDB/falkordb-rs#48
File: src/parser/mod.rs:82-86
Timestamp: 2024-10-13T19:43:33.128Z
Learning: When reviewing code in the FalkorDB Rust client, if `FalkorDBError` variants seem undefined, check if they are added in the same PR before flagging them as missing.
Applied to files:
src/client/blocking.rssrc/parser/mod.rs
📚 Learning: 2024-10-10T11:45:20.321Z
Learnt from: barakb
PR: FalkorDB/falkordb-rs#48
File: Cargo.toml:27-29
Timestamp: 2024-10-10T11:45:20.321Z
Learning: In the `falkordb` project, the `approx` crate is used in `src/client/blocking.rs` only within test code under `#[cfg(test)]`, so it's acceptable for it to be a development dependency.
Applied to files:
src/client/blocking.rssrc/parser/mod.rs
🧬 Code graph analysis (3)
src/value/mod.rs (1)
src/parser/mod.rs (1)
try_from(38-58)
src/client/blocking.rs (2)
src/lib.rs (1)
create_test_client(87-91)src/value/mod.rs (8)
as_time(213-218)as_date(203-208)as_date_time(193-198)from(82-84)from(88-90)from(94-96)from(100-102)from(106-108)
src/parser/mod.rs (2)
src/graph_schema/mod.rs (2)
new(87-99)open_readonly_graph_with_modified_schema(284-303)src/client/blocking.rs (1)
create_empty_inner_sync_client(241-250)
🪛 GitHub Actions: Code Coverage
src/parser/mod.rs
[warning] 109-109: function parse_duration_from_string is never used
🔇 Additional comments (14)
src/client/blocking.rs (7)
7-11: LGTM: import re-org only.No functional impact; ordering is fine.
Also applies to: 15-16
258-262: LGTM: test imports.Reusing create_test_client and adding chrono traits is appropriate.
321-326: LGTM: clearer failure message.Using {e} improves diagnostics.
344-345: LGTM: iterator rewrite.Using by_ref() with a for-loop is cleaner and equivalent.
366-385: LGTM: time parsing test.Good direct assertions on hour/minute/second.
386-403: LGTM: date parsing test.Covers year extraction correctly.
404-421: LGTM: datetime parsing test.Asserting year is sufficient here.
src/value/mod.rs (5)
48-55: LGTM: new temporal variants.Clear, minimal additions to the public enum.
87-109: LGTM: Fromchrono::* impls.Straightforward and consistent with existing conversions.
189-229: LGTM: temporal accessors.Naming and return types are consistent.
478-537: LGTM: tests for accessors and From/TryFrom.Covers the happy paths well.
538-543: LGTM: error path test.Validates ParseTemporalError surface.
src/parser/mod.rs (2)
29-33: LGTM: type marker extensions and mapping.Values 13–16 routed correctly.
Also applies to: 52-56
443-463: Confirm timestamp unit and add bounds checks (minor).Assumes seconds precision; consider documenting and returning clearer errors for out-of-range inputs.
Optionally:
- DateTime::<Utc>::from_timestamp(redis_value_as_int(val)?, 0) + let secs = redis_value_as_int(val)?; + DateTime::<Utc>::from_timestamp(secs, 0)Add a brief doc comment clarifying seconds precision and behavior on overflow.
| #[test] | ||
| fn test_parse_duration_simple() { | ||
| let mut graph = open_readonly_graph_with_modified_schema(); | ||
|
|
||
| let res = parse_type( | ||
| ParserTypeMarker::Duration, | ||
| redis::Value::SimpleString("P1Y2M3DT4H5M6S".to_string()), | ||
| graph.get_graph_schema_mut(), | ||
| ); | ||
| assert!(res.is_ok()); | ||
|
|
||
| let falkor_duration = res.unwrap(); | ||
| let FalkorValue::Duration(duration) = falkor_duration else { | ||
| panic!("Is not of type duration") | ||
| }; | ||
|
|
||
| // Should be approximately 1 year + 2 months + 3 days + 4 hours + 5 minutes + 6 seconds | ||
| // = 365*24*3600 + 60*24*3600 + 3*24*3600 + 4*3600 + 5*60 + 6 seconds | ||
| let expected_seconds = | ||
| 365 * 24 * 3600 + 60 * 24 * 3600 + 3 * 24 * 3600 + 4 * 3600 + 5 * 60 + 6; | ||
| assert_eq!(duration.num_seconds(), expected_seconds); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_parse_duration_time_only() { | ||
| let mut graph = open_readonly_graph_with_modified_schema(); | ||
|
|
||
| let res = parse_type( | ||
| ParserTypeMarker::Duration, | ||
| redis::Value::SimpleString("PT2H30M15S".to_string()), | ||
| graph.get_graph_schema_mut(), | ||
| ); | ||
| assert!(res.is_ok()); | ||
|
|
||
| let falkor_duration = res.unwrap(); | ||
| let FalkorValue::Duration(duration) = falkor_duration else { | ||
| panic!("Is not of type duration") | ||
| }; | ||
|
|
||
| // Should be 2 hours + 30 minutes + 15 seconds = 2*3600 + 30*60 + 15 = 9015 seconds | ||
| assert_eq!(duration.num_seconds(), 9015); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_parse_duration_date_only() { | ||
| let mut graph = open_readonly_graph_with_modified_schema(); | ||
|
|
||
| let res = parse_type( | ||
| ParserTypeMarker::Duration, | ||
| redis::Value::SimpleString("P1Y6M".to_string()), | ||
| graph.get_graph_schema_mut(), | ||
| ); | ||
| assert!(res.is_ok()); | ||
|
|
||
| let falkor_duration = res.unwrap(); | ||
| let FalkorValue::Duration(duration) = falkor_duration else { | ||
| panic!("Is not of type duration") | ||
| }; | ||
|
|
||
| // Should be 1 year + 6 months = 365*24*3600 + 6*30*24*3600 seconds | ||
| let expected_seconds = 365 * 24 * 3600 + 6 * 30 * 24 * 3600; | ||
| assert_eq!(duration.num_seconds(), expected_seconds); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_parse_duration_invalid() { | ||
| let mut graph = open_readonly_graph_with_modified_schema(); | ||
|
|
||
| let res = parse_type( | ||
| ParserTypeMarker::Duration, | ||
| redis::Value::SimpleString("INVALID".to_string()), | ||
| graph.get_graph_schema_mut(), | ||
| ); | ||
| assert!(res.is_err()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_parse_duration_from_string_function() { | ||
| // Test the parse_duration_from_string function directly | ||
| let duration = parse_duration_from_string("P1DT2H3M4S").unwrap(); | ||
| let expected_seconds = 1 * 24 * 3600 + 2 * 3600 + 3 * 60 + 4; | ||
| assert_eq!(duration.num_seconds(), expected_seconds); | ||
|
|
||
| // Test with P prefix | ||
| let duration2 = parse_duration_from_string("P1DT2H3M4S").unwrap(); | ||
| assert_eq!(duration2.num_seconds(), expected_seconds); | ||
|
|
||
| // Test without P prefix | ||
| let duration3 = parse_duration_from_string("1DT2H3M4S").unwrap(); | ||
| assert_eq!(duration3.num_seconds(), expected_seconds); | ||
|
|
||
| // Test invalid input | ||
| let result = parse_duration_from_string("INVALID"); | ||
| assert!(result.is_err()); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Duration tests will fail until Duration string support is added.
These rely on SimpleString inputs; blocked on the parse_type change above.
To confirm coverage after the fix, check that the helper is used:
🏁 Script executed:
#!/bin/bash
# Verify parse_duration_from_string is referenced by parse_type
rg -n "parse_duration_from_string\(" -C2Length of output: 1605
Add Duration support in parse_type
In src/parser/mod.rs, extend the ParserTypeMarker::Duration branch to handle redis::Value::SimpleString(s) by invoking parse_duration_from_string(&s)? and returning FalkorValue::Duration(duration), and return an error for all other variants.
🤖 Prompt for AI Agents
In src/parser/mod.rs around lines 877 to 971, the ParserTypeMarker::Duration arm
needs to handle redis::Value::SimpleString by calling
parse_duration_from_string(&s)? and wrapping the result in
FalkorValue::Duration(duration); for any other redis::Value variants return a
parse/type error. Update the match for the Duration marker to pattern-match
SimpleString(s) -> let duration = parse_duration_from_string(&s)?;
Ok(FalkorValue::Duration(duration)) and ensure other value patterns map to Err
with an appropriate parse error.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
1 (4)
3-3: If using captured identifiers in format strings, declare MSRVFormat string capture (e.g., format!("{x:?}")) requires a minimum Rust version; declare it to avoid surprises for downstreams.
Proposed Cargo.toml snippet (pick the correct MSRV your code actually requires):
[package] name = "falkordb-rs" version = "…" edition = "2021" +rust-version = "1.58" # or higher if other features require it (2024 edition implies a higher MSRV)
4-4: Public API expansion: confirm stability or gate via prelude/doc-hiddenChanging pub(crate) → pub grows the stable surface; walking this back is semver-breaking. If not ready to commit, re-export via a prelude or mark items doc(hidden).
Example patterns:
- pub struct InternalHelper { /* … */ } + #[doc(hidden)] + pub struct InternalHelper { /* … */ } - // scattered pub items +pub mod prelude { + pub use crate::{FalkorValue, QueryResult /* curated set only */}; +}Consider a minor version bump (or clear “unstable” docs) if you intend to iterate.
5-5: Harden docs: enable broken intra-doc link checksLarge doc edits risk link rot; enforce at crate root.
+#![deny(rustdoc::broken_intra_doc_links)] +#![warn(rustdoc::missing_crate_level_docs)]
6-6: Codify literal/style cleanups with lints to keep them consistentEnable Clippy lints so future diffs stay clean without churn.
+#![warn(clippy::unreadable_literal, clippy::inconsistent_digit_grouping, clippy::approx_constant)]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
1(1 hunks)deny.toml(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: gkorland
PR: FalkorDB/falkordb-rs#48
File: src/parser/mod.rs:82-86
Timestamp: 2024-10-13T19:43:33.128Z
Learning: When reviewing code in the FalkorDB Rust client, if `FalkorDBError` variants seem undefined, check if they are added in the same PR before flagging them as missing.
🔇 Additional comments (5)
deny.toml (3)
13-18: Verify ring license hash
Cargo-deny couldn’t run here (“Could not locate working directory: no /proc/self/exe available”). Runcargo deny check licenses -din CI or a local environment with a mounted /proc to confirm the hash matches your locked ring version. Optionally, reduce maintenance by adding
version = "<locked-semver>"to this clarification. deny.toml 13–18
6-6: Use structured skip entries and retain windows-linkReplace the skip list in deny.toml with:
skip = [ { name = "windows-link" }, # valid crate in Cargo.lock { name = "windows-sys" }, # present in Cargo.lock { name = "windows-targets" } # present in Cargo.lock ]Likely an incorrect or invalid review comment.
20-23: Pin cargo-deny to a version supporting the new[licenses]keys
Bothunused-allowed-licenseandconfidence-thresholdrequire cargo-deny versions that include them—verify and pin your CI’s cargo-deny to avoid config-parse failures.1 (2)
1-1: All #[must_use] annotations are appropriately applied
No functions returning()or&mut Self, nor free functions returningbool/Option, are annotated without a message—no changes needed.
2-2: Docs verified:# Errorsusage correct
No# Errorssections on functions without aResultreturn type.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
deny.toml (1)
27-33: Consider dropping the LICENSE hash to avoid brittle failures on ring updates.Pinning the file hash is precise but will break on any upstream LICENSE whitespace tweak. If you prefer stability across minor ring releases, omit the hash; cargo-deny will still use the clarification.
Example:
[[licenses.clarify]] name = "ring" expression = "MIT AND ISC AND OpenSSL" license-files = [ - { path = "LICENSE", hash = 0xbd0eed23 } + { path = "LICENSE" } ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
deny.toml(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: gkorland
PR: FalkorDB/falkordb-rs#48
File: src/parser/mod.rs:82-86
Timestamp: 2024-10-13T19:43:33.128Z
Learning: When reviewing code in the FalkorDB Rust client, if `FalkorDBError` variants seem undefined, check if they are added in the same PR before flagging them as missing.
🔇 Additional comments (2)
deny.toml (2)
34-36: LGTM on new licenses fields."unused-allowed-license" and "confidence-threshold" keys match cargo-deny docs; settings are reasonable.
6-20: Verify skipped crates are truly duplicated
- Confirm
windows-linkappears in multiple versions before whitelisting it.- Update comments for
core-foundationandsecurity-framework(they’re macOS-only).- Clarify why
wasiis listed—it's not Windows-specific.- To reduce noise, consider constraining duplicate checks to specific OS targets via a
[graph.targets]list.
| [[licenses.clarify]] | ||
| name = "ring" | ||
| expression = "MIT AND ISC AND OpenSSL" | ||
| license-files = [ | ||
| { path = "LICENSE", hash = 0xbd0eed23 } | ||
| ] | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add OpenSSL to the allowed licenses or this clarify will fail license checks.
You clarified ring as "MIT AND ISC AND OpenSSL", but [licenses.allow] below doesn’t include "OpenSSL". cargo-deny will reject ring unless "OpenSSL" is allowed. Fix by adding it to the allow list.
Apply:
[licenses]
unused-allowed-license = "allow"
confidence-threshold = 0.93
allow = [
"Apache-2.0 WITH LLVM-exception",
"Apache-2.0",
"BSD-2-Clause",
"BSD-3-Clause",
"MIT",
"ISC",
+ "OpenSSL",
"Unicode-3.0",
]Run:
Also applies to: 37-45
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Validate config parses and licenses pass (requires cargo-deny installed)
cargo deny check licensesLength of output: 109
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate deny.toml and inspect the [licenses] allow list
file=$(find . -maxdepth 2 -type f -name 'deny.toml' | head -n1)
echo "Inspecting [licenses] block in $file"
awk '/^\[licenses\]/{p=1} p; /^\[/{if(p && $0!~/^\[licenses\]/) exit}' "$file"
echo
echo "Checking for presence of 'OpenSSL' in allow list"
grep -n '"OpenSSL"' "$file" || echo "License 'OpenSSL' not found"Length of output: 223
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=$(find . -maxdepth 2 -type f -name 'deny.toml' | head -n1)
echo "[licenses] section from $file"
sed -n '/^\[licenses\]/,/^\[/{p}' "$file"
echo
echo "Searching for \"OpenSSL\" in the allow list"
grep -n '"OpenSSL"' "$file" || echo "License 'OpenSSL' not found in allow list"Length of output: 341
Add “OpenSSL” to the [licenses].allow list
The clarify rule for ring includes “OpenSSL”, but the allow array in deny.toml currently omits it, causing cargo-deny to fail. Apply:
allow = [
"Apache-2.0 WITH LLVM-exception",
"Apache-2.0",
"BSD-2-Clause",
"BSD-3-Clause",
"MIT",
"ISC",
+ "OpenSSL",
"Unicode-3.0",
]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [[licenses.clarify]] | |
| name = "ring" | |
| expression = "MIT AND ISC AND OpenSSL" | |
| license-files = [ | |
| { path = "LICENSE", hash = 0xbd0eed23 } | |
| ] | |
| allow = [ | |
| "Apache-2.0 WITH LLVM-exception", | |
| "Apache-2.0", | |
| "BSD-2-Clause", | |
| "BSD-3-Clause", | |
| "MIT", | |
| "ISC", | |
| "OpenSSL", | |
| "Unicode-3.0", | |
| ] |
🤖 Prompt for AI Agents
In deny.toml around lines 27 to 33, the clarify rule for the "ring" license
expression includes "OpenSSL" but the top-level [licenses].allow array does not
include "OpenSSL", causing cargo-deny to fail; fix this by adding the string
"OpenSSL" to the licenses.allow array (matching exact capitalization) so the
clarify rule is permitted.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores