-
Notifications
You must be signed in to change notification settings - Fork 3
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
bump: hdk 3.1 hdi 4.1 #95
base: main
Are you sure you want to change the base?
Conversation
@@ -33,15 +33,17 @@ pub fn delete_trust_atoms(target: AnyLinkableHash) -> ExternResult<DeleteReport> | |||
let agent_pubkey = agent_info()?.agent_initial_pubkey; | |||
|
|||
// Forward Links | |||
let forward_links = get_links(agent_pubkey.clone(), LinkTypes::TrustAtom, None)?; | |||
let forward_links = | |||
get_links(GetLinksInputBuilder::try_new(agent_pubkey.clone(), LinkTypes::TrustAtom)?.build())?; |
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.
hm, this seems a bit ugly... @dauphin3 what do you think of introducing a util function somewhere that has the old signature? so we would say:
util::get_links(agent_pubkey.clone(), LinkTypes::TrustAtom, None)?;
and it would call this new code under the hood?
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.
Then a bunch of the changes in this PR would reduce to just adding util::
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.
@harlantwood
in v3 get_links now takes a single parameter in the form of GetLinksInput and so
https://docs.rs/hdk/0.3.1/hdk/link/builder/struct.GetLinksInputBuilder.html defaults GetOptions and sets other options to None:
pub struct GetLinksInput {
pub base_address: HoloHash<AnyLinkable>,
pub link_type: LinkTypeFilter,
pub get_options: GetOptions,
pub tag_prefix: Option<LinkTag>,
pub after: Option<Timestamp>,
pub before: Option<Timestamp>,
pub author: Option<HoloHash<Agent>>,
}
thus for a utility function we would still need to handle optional parameters if using the builder, however a different potential change could be to leave the GetLinksInput struct in the pass through, for example:
get_links(
GetLinksInput {
pub base_address: agent_pubkey,
pub link_type: LinkTypes::TrustAtom,
pub get_options: GetOptions::default(),
pub tag_prefix: None,
pub after: None,
pub before: None,
pub author: None,
}
while not necessarily cleaner, it would be more explicit
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.
thus for a utility function we would still need to handle optional parameters if using the builder
not necessarily, if all of our usages of the function follow the same pattern and we don’t have to allow options in the utility function signature.
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 agree it would make things a bit more cleaner for now, however I lean toward keeping the builder pattern for future flexibility, since it makes it convenient to chain additional parameters if we ever do decide to add query parameters, as well as it is recommended in the migration guide. I think we may end up wanting to leverage the new ways to filter a get_links query at some point and seems like we might use it eventually anyway. (I could see the 'before' & 'after' options coming in handy for filtering timestamps [eg. MewsFeed use-case] )
link_tag is already an optional parameter and the some case is used here:
let links = get_links(link_base.clone(), LinkTypes::TrustAtom, link_tag)?; |
If I were to proceed as recommended, I think it might make sense to leave the builder out of the util function and just pass through the variables into the GetLinksInput struct as seen above with the given defaults, so as not to have to match on the link_tag option
otherwise the util function would like like this:
pub fn get_links(
base: AnyLinkableHash,
link_type: LinkTypes,
link_tag: Option<LinkTag>,
) -> ExternResult<Vec<Link>> {
if let Some(link_tag) = link_tag {
let links = hdk::link::get_links(
GetLinksInputBuilder::try_new(base, link_type)?
.tag_prefix(link_tag)
.build(),
)?;
Ok(links)
} else {
let links = hdk::link::get_links(GetLinksInputBuilder::try_new(base, link_type)?.build())?;
Ok(links)
}
}
which isn't bad, but could get lengthy if the signature changes to add more options
if to proceed with util function, should it be called get_links ? or something like get_links_default_options ?
or maybe get_links_for_trust_atom ? ..
recommended release Holochain v3: https://blog.holochain.org/holochain-0-3-a-new-launcher-and-hc-on-mobile/
migration guide, for reference: https://developer.holochain.org/resources/upgrade/upgrade-holochain-0.3/