Unnecessarily Cloning, structuring #1905
Conversation
Redundant Option Checks, unwrap Safety, unnecessary Lifetimes, Rust often infers lifetimes automatically
src/options/set.rs
Outdated
| @@ -79,7 +79,7 @@ pub fn set_options( | |||
| } | |||
| opt.navigate = opt.navigate || opt.env.navigate.is_some(); | |||
| if opt.syntax_theme.is_none() { | |||
| opt.syntax_theme.clone_from(&opt.env.bat_theme); | |||
| opt.syntax_theme = opt.env.bat_theme.take(); | |||
There was a problem hiding this comment.
While there might be an efficiency saving here, it makes the data inconsistent, so I think we should only make changes like that for demonstrated performance wins.
There was a problem hiding this comment.
oh, i see, thanks
src/git_config/mod.rs
Outdated
| .get(2) | ||
| .or_else(|| captures.get(4)) | ||
| .map_or("".to_string(), |m| m.as_str().to_string()); | ||
| (key, value) |
There was a problem hiding this comment.
Would you mind reverting this change? It loses the "joint distribution" of the data: i.e. it's not as obvious from the new code that, for example, they will either both be 0 or neither be 0.
There was a problem hiding this comment.
sure, corrected
src/handlers/blame.rs
Outdated
| let commit = caps.get(1).unwrap().as_str(); | ||
| let author = caps.get(2).unwrap().as_str(); | ||
| let timestamp = caps.get(3).unwrap().as_str(); | ||
| let commit = caps.get(1)?.as_str(); | ||
| let author = caps.get(2)?.as_str(); | ||
| let timestamp = caps.get(3)?.as_str(); | ||
|
|
||
| let time = DateTime::parse_from_str(timestamp, timestamp_format).ok()?; | ||
|
|
||
| let line_number = caps.get(4).unwrap().as_str().parse::<usize>().ok()?; | ||
| let line_number = caps.get(4)?.as_str().parse::<usize>().ok()?; | ||
|
|
||
| let code = caps.get(5).unwrap().as_str(); |
There was a problem hiding this comment.
unwrap() here is actually okay because the regex is know at compile time. If it is changed but not the corresponding code here then it should crash for the developer and not silently fail and return None.
Similarly for the max line numbers above.
There was a problem hiding this comment.
unwrap()here is actually okay because the regex is know at compile time. If it is changed but not the corresponding code here then it should crash for the developer and not silently fail and return None. Similarly for the max line numbers above.
should we reverting this changes?
There was a problem hiding this comment.
Yes, I think the only bit of the PR that arguably should be retained is not using else after the early return in src/align.rs -- I do slightly prefer that. But beyond that, there are lots of bugs and feature requests to work on if you've got time! And any performance improvements backed by profiling of course would be welcome.
There was a problem hiding this comment.
We could create and tag issues as 🟣 good first issue for that.
There was a problem hiding this comment.
Thanks for the review, corrected
Hello, minor updates, unnecessarily cloning, structuring