-
Notifications
You must be signed in to change notification settings - Fork 267
Conversation
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.
Can you please check my comments below? I believe this code would return the headers of the link targets but not of the links, if I'm not mistaken.
…ochain-rust into get-links-crud
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.
Cool!
@@ -150,7 +150,7 @@ pub enum Action { | |||
/// Last string is the stringified process unique id of this `hdk::get_links` call. | |||
GetLinks(GetLinksKey), | |||
GetLinksTimeout(GetLinksKey), | |||
RespondGetLinks((FetchMetaData, Vec<Address>)), | |||
RespondGetLinks((FetchMetaData, Vec<(Address, CrudStatus)>)), |
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.
@AshantiMutinta Is a tuple future proof enough? Do you imagine other stuff getting associated with address here?
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 think for now it works because serialization will save space because no field name values. When it grows, should definitely use a struct
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 think for now it works because serialization will save space because no field name values. When it grows, should definitely use a struct
use holochain_net::connection::json_protocol::{FetchMetaData, FetchMetaResultData, JsonProtocol}; | ||
|
||
/// Send back to network a HandleFetchMetaResult, no matter what. | ||
/// Will return an empty content field if it actually doesn't have the data. | ||
fn reduce_respond_get_links_inner( | ||
network_state: &mut NetworkState, | ||
get_dht_meta_data: &FetchMetaData, | ||
links: &Vec<Address>, | ||
links: &Vec<(Address, CrudStatus)>, |
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.
Similar to other comment, is a tuple good enough here for immediate future?
(links[0] == (entry_addresses[1].clone(), CrudStatus::Live) | ||
|| links[0] == (entry_addresses[2].clone(), CrudStatus::Live)) | ||
&& (links[1] == (entry_addresses[1].clone(), CrudStatus::Live) | ||
|| links[1] == (entry_addresses[2].clone(), CrudStatus::Live)) |
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 test is getting heavy. If it fails it would be probably be a pain to debug. I think it's outside the scope of your PR to fix it however.
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.
Yes it would be nice to break out in follow up pr
PR summary
-This PR allows for obtaining crud information with the get_links. The crud information is implied from the link type and we can query this information based on LinkStatusRequestKind
testing/benchmarking notes
Testing of this was implemented by modification and addition of app_spec and united tests.
followups
-One of the tests in the intergration suite uses a string which makes it hard to debug, we should switch to a struct.
-We should offset the querying of the link to the node end rather than the client. This would mean modifying some of the structs on the network side
Please check one of the following, relating to the CHANGELOG
- summary of change [PR#1234](https://github.com/holochain/holochain-rust/pull/1234)