chore: table overflow fixed #66
Conversation
WalkthroughThe changes introduce a new function, Changes
Sequence Diagram(s)sequenceDiagram
participant TT as to_table
participant TS as truncate_string
participant Record as Record String
Record->>TT: Pass string value
TT->>TT: Check if string length > 20
alt Long String
TT->>TS: Call truncate_string(input, 4, 3)
TS-->>TT: Return truncated string
else
TT-->>TT: Use original string
end
TT->>Record: Return processed string for display
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/cli/src/main.rs (1)
86-91: Extract magic numbers into named constants.The hardcoded values for string length threshold and truncation parameters should be constants at the module level for better maintainability.
+const MAX_FIELD_LENGTH: usize = 20; +const PREFIX_LENGTH: usize = 4; +const SUFFIX_LENGTH: usize = 3; for record in reader.into_records() { let record = record?; let iter = record.iter().map(|s| { - if s.len() > 20 { - return truncate_string(s, 4, 3) + if s.len() > MAX_FIELD_LENGTH { + return truncate_string(s, PREFIX_LENGTH, SUFFIX_LENGTH) } s.to_owned() });
| fn truncate_string(input: &str, prefix_len: usize, suffix_len: usize) -> String { | ||
| if input.len() <= prefix_len + suffix_len { | ||
| return input.to_string(); // No need to truncate | ||
| } | ||
|
|
||
| format!("{}...{}", &input[..prefix_len], &input[input.len() - suffix_len..]) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and tests for the truncation function.
The function needs validation and test coverage:
- Validate that prefix_len + suffix_len > 0
- Handle cases where prefix_len + suffix_len > input.len()
- Add unit tests as mentioned in PR checklist
fn truncate_string(input: &str, prefix_len: usize, suffix_len: usize) -> String {
+ // Validate input parameters
+ if prefix_len == 0 && suffix_len == 0 {
+ return input.to_string();
+ }
+
+ // Ensure we don't panic on short inputs
+ let total_len = prefix_len.saturating_add(suffix_len);
if input.len() <= prefix_len + suffix_len {
return input.to_string(); // No need to truncate
}
format!("{}...{}", &input[..prefix_len], &input[input.len() - suffix_len..])
}Consider adding these test cases:
#[cfg(test)]
mod tests {
use super::*;
#[test]
fn test_truncate_string() {
assert_eq!(truncate_string("abcdefghijk", 4, 3), "abcd...ijk");
assert_eq!(truncate_string("short", 4, 3), "short");
assert_eq!(truncate_string("a", 4, 3), "a");
assert_eq!(truncate_string("", 4, 3), "");
}
}
|
@iankressin please merge |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/cli/src/main.rs (2)
86-91: Extract magic numbers as constants.The hardcoded values should be constants for better maintainability and reusability.
+const MAX_FIELD_LENGTH: usize = 20; +const PREFIX_LENGTH: usize = 4; +const SUFFIX_LENGTH: usize = 3; + let iter = record.iter().map(|s| { - if s.len() > 20 { - return truncate_string(s, 4, 3); + if s.len() > MAX_FIELD_LENGTH { + return truncate_string(s, PREFIX_LENGTH, SUFFIX_LENGTH); } s.to_owned() });Also, consider adding validation to ensure
PREFIX_LENGTH + SUFFIX_LENGTH < MAX_FIELD_LENGTH.
143-154: Consider adding more edge cases to tests.While the current test coverage is good, consider adding these edge cases:
#[test] fn test_truncate_string() { assert_eq!(truncate_string("abcdefghijk", 4, 3), "abcd...ijk"); assert_eq!(truncate_string("short", 4, 3), "short"); assert_eq!(truncate_string("a", 4, 3), "a"); assert_eq!(truncate_string("", 4, 3), ""); + // Edge cases + assert_eq!(truncate_string("test", 0, 0), "test"); + assert_eq!(truncate_string("abcdef", 3, 2), "abc...ef"); + assert_eq!(truncate_string("abc", 2, 2), "abc"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
crates/cli/src/main.rs(2 hunks)crates/core/src/common/logs.rs(1 hunks)crates/core/src/interpreter/backend/execution_engine.rs(12 hunks)crates/core/src/interpreter/backend/mod.rs(1 hunks)crates/macros/src/lib.rs(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- crates/macros/src/lib.rs
- crates/core/src/interpreter/backend/execution_engine.rs
- crates/core/src/interpreter/backend/mod.rs
🔇 Additional comments (2)
crates/cli/src/main.rs (1)
127-140: Implementation looks good!The function handles edge cases well and includes proper input validation.
crates/core/src/common/logs.rs (1)
145-146: LGTM! Good cleanup.Removing the unnecessary
Noneargument simplifies the code without affecting functionality.
|
@iankressin i am done with this , if you cool with it |
1 participant
Summary
Truncated fields for long fields, instead of printing a really long string, we should truncate it like 0x124...123 fixed the issue
Before
After
Related Issue
#60
Technical details
Truncating long strings fixed it
Topics for Discussion
PR Checklist
Screenshots (if applicable)
Summary by CodeRabbit
New Features
Bug Fixes
Style
Chores