Refactoring fetch_bookmark to avoid extra queries #1306
Conversation
|
Hi @thomcc, it seems |
|
Retriggered, I agree it seems unrelated. Just FYI, most of the team is traveling for a conference so review turnaround might be slightly slower this week. |
There was a problem hiding this comment.
Hi @lyuyuan92, thanks so much for the patch, and apologies for the delay in reviewing! This looks fantastic! The only blocking question I had is using the level that we get back from the query, instead of keeping track of it ourselves. I wouldn't mind having another look, so requesting changes, but this is in pretty good shape for merging soon! 🚀🚢
| } | ||
| None => return Ok(None), | ||
| }; | ||
|
|
||
| // Skip the rest and return if root is not a folder | ||
| match root { |
There was a problem hiding this comment.
Nit: Or-patterns in if-let are stabilized in Rust 1.33; you might be able to use if let BookmarkTreeNode::Bookmark(_) | BookmarkTreeNode::Separator(_) = root here.
| // Keep track of folders' guid of each level. Initialize the vector with two | ||
| // sets, which are for the root and its immediate children folders. | ||
| let mut node_vec: Vec<HashSet<SyncGuid>> = vec![HashSet::new(); 2]; | ||
| let mut curr_depth: usize = 0; |
There was a problem hiding this comment.
It looks like the query already selects the level, and we can access it via row.level. Do you think we can replace node_vec and curr_depth with a check like row.level > d + 1 in the loop below?
There was a problem hiding this comment.
Ah, I am a bit silly here...totally forgot about the level field. Done.
| return Ok(if get_direct_children { | ||
| ChildInfo::Nodes(vec![]) | ||
| fn set_child_guids_and_drop(parent: &mut PublicNode) { | ||
| if let Some(children) = parent.child_nodes.take() { |
There was a problem hiding this comment.
Nit: You can make this a one-liner with a couple of maps, if you like. 😄
parent.child_guids = parent
.child_nodes
.take()
.map(|children| children.into_iter().map(|child| child.guid).collect());...And even inline that into fetch_bookmark, since it's pretty short. But I'm also totally fine with keeping it as a separate function, if you think it's more readable.
There was a problem hiding this comment.
Done. The suggested code is much cleaner, thanks!
1. Allow a depth parameter to be passed into fetch_tree. 2. Refact fetch bookmark functions.
fixes #806