-
Notifications
You must be signed in to change notification settings - Fork 39
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
feat: add merge feature #155
Conversation
7753a02
to
141a21c
Compare
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.
I haven't checked the correctness of what this is doing yet, but I think the code can be streamlined quite a lot.
src/db/entry.rs
Outdated
@@ -41,6 +41,44 @@ impl Entry { | |||
..Default::default() | |||
} | |||
} | |||
|
|||
pub(crate) fn merge(&self, other: &Entry) -> Entry { |
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.
You are using unwrap()
so often, this method should return a Result
type instead.
src/db/entry.rs
Outdated
if destination_modification_time > source_modification_time { | ||
response = self.clone(); | ||
// TODO we could just return if the other entry doesn't have a history. | ||
let mut source_history = other.history.clone().unwrap_or(History::default()); |
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.
See comment above, then
let mut source_history = other.history.clone().unwrap_or(History::default()); | |
let mut source_history = other.history.clone().ok_or("No history")?; |
src/db/entry.rs
Outdated
// TODO we could just return if the other entry doesn't have a history. | ||
let mut source_history = other.history.clone().unwrap_or(History::default()); | ||
source_history.add_entry(other.clone()); | ||
let mut new_history = self.history.clone().unwrap_or(History::default()).clone(); |
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.
let mut new_history = self.history.clone().unwrap_or(History::default()).clone(); | |
let mut new_history = self.history.clone().unwrap_or_else(|| History::default()).clone(); |
src/db/entry.rs
Outdated
let mut destination_history = self.history.clone().unwrap_or(History::default()); | ||
destination_history.add_entry(self.clone()); | ||
let mut new_history = other.history.clone().unwrap_or(History::default()).clone(); |
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.
See above.
|
||
// Determines if the entries of the history are | ||
// ordered by last modification time. | ||
pub(crate) fn is_ordered(&self) -> bool { |
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.
This should probably be Option<bool>
, then you can save a lot of boilerplate in the function body.
for node in &mut self.children { | ||
if let Node::Entry(e) = node { | ||
response.push(e) | ||
} | ||
} |
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.
for node in &mut self.children { | |
if let Node::Entry(e) = node { | |
response.push(e) | |
} | |
} | |
self.children.iter_mut().flat_map(|node| { | |
match node { | |
Node::Entry(e) => Some(e), | |
_ => None | |
} | |
}).collect() |
for node in &mut self.children { | ||
if let Node::Group(g) = node { | ||
response.push(g); | ||
} | ||
} |
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.
for node in &mut self.children { | |
if let Node::Group(g) = node { | |
response.push(g); | |
} | |
} | |
self.children.iter_mut().flat_map(|node| { | |
match node { | |
Node::Group(g) => Some(g), | |
_ => None | |
} | |
}).collect() |
for node in &self.children { | ||
match node { | ||
Node::Group(g) => { | ||
if let Some(e) = g.find_entry_by_uuid(id) { | ||
return Some(e); | ||
} | ||
} | ||
Node::Entry(e) => { | ||
if e.uuid == id { | ||
return Some(e); | ||
} | ||
} | ||
} | ||
} | ||
None |
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.
You should probably implement a recursive iterator, so you can do this:
for node in &self.children { | |
match node { | |
Node::Group(g) => { | |
if let Some(e) = g.find_entry_by_uuid(id) { | |
return Some(e); | |
} | |
} | |
Node::Entry(e) => { | |
if e.uuid == id { | |
return Some(e); | |
} | |
} | |
} | |
} | |
None | |
self.iter_recursive() | |
.flat_map(|c| { | |
match c { | |
Node::Entry(e) => Some(e), | |
_ => None | |
} | |
}) | |
.find(|e| e.uuid == id) |
} | ||
|
||
pub fn find_entry_by_uuid(&self, id: Uuid) -> Option<&Entry> { | ||
for node in &self.children { |
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.
for node in &self.children { | |
self.children.iter_mut().for_each(|node| { |
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.
Not sure what's the advantage of this proposition. We don't need a mutable reference to the node, and the proposed syntax is more verbose.
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.
The mut was a bit of an oversight. iter() should be sufficient. Overall, the iterator syntax should be preferred. It's cleaner and often faster than the for-loop.
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.
often faster than the for-loop.
Do you have a source for that? I would like to know why that's the case.
It's cleaner
I personally prefer the for syntax, especially in Rust. Notice the absence of parentheses on the line where the for
is declared ✨
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.
Do you have a source for that? I would like to know why that's the case.
The iterators are usually as fast, sometimes faster. The difference isn't huge, but I guess it comes mostly from Rust's automatic SIMD vectorization. The same happens for for loops, but I guess they are sometimes not as easy to optimize.
I personally prefer the for syntax, especially in Rust. Notice the absence of parentheses on the line where the for is declared
I would say the functional approach is definitely more idiomatic, especially if you have loops without side effects. This is different in Javascript, where iterators are eagerly executed, and so you loop over your data multiple times if you do filter().map()
, but not in Rust. Here I would avoid explicit for loops if possible and also avoid side effects if possible, which for loops tend to invite you to introduce.
To be honest, the for_each() iterator was lazy by me. You could rewrite the whole function like that:
pub fn find_entry_by_uuid(&self, id: Uuid) -> Option<&Entry> {
self.children.iter().flat_map(|node| {
match node {
Node::Group(g) => g.find_entry_by_uuid(id),
Node::Entry(e) => Some(e)
}
}).find(|entry| entry.uuid == id)
}
Not only shorter, but also easier to read and understand. You don't come up with this kind of solution if you think in for loops, though.
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.
Another way to write it:
pub fn find_entry_by_uuid(&self, id: Uuid) -> Option<&Entry> {
self.children.iter().flat_map(|node| {
match node {
Node::Group(g) => g.find_entry_by_uuid(id),
Node::Entry(e) => if e.uuid == id {
Some(e)
} else {
None
}
}
}).next()
}
It's slightly longer, but avoids comparing uuids multiple times when walking up the recursion chain. Not sure if it makes a huge difference.
pub(crate) fn get_all_entries( | ||
&self, | ||
current_location: &NodeLocation, | ||
) -> Vec<(&Entry, NodeLocation)> { |
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.
This should be replaced with a recursive iterator implementation as suggested above.
I read through the code a bit more. From what I understood, the |
7e5ff35
to
bf178e6
Compare
3c17dba
to
39f64a2
Compare
bbd2eed
to
9a878d8
Compare
9a878d8
to
31f6aa4
Compare
31f6aa4
to
8bc70d3
Compare
8bc70d3
to
4b29d74
Compare
Superseded by #201 |
This is a very early version of a merge feature. KeePassXC defines 5 merge modes, but I don't think we need all the 5 modes in the first version of this feature, if ever
TODOs