-
Notifications
You must be signed in to change notification settings - Fork 267
Adds regex matching for link tags and types #1453
Conversation
core/src/dht/dht_store.rs
Outdated
@@ -62,16 +63,20 @@ impl DhtStore { | |||
link_type: Option<String>, | |||
tag: Option<String>, | |||
) -> Result<BTreeSet<EntityAttributeValueIndex>, HolochainError> { | |||
// interpret link tags and types as regex | |||
let link_type = link_type.map(|s| Regex::new(&s).unwrap()); |
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.
Should improve handling of invalid regex and ensure the error happens in the calling code
It's really exciting to see this being explored already! I'm just chewing on it from a zome developer's perspective... I feel like having it as a string field that always gets interpreted as a regex puts a burden on the developer to remember to put start/end anchors on and escape special chars when they want an exact tag match. I don't know if there's an idiomatic Rust way of specifying either string or regex, but I feel like there must be. In lieu of that, how about something like enum LinksTagQuery {
Any,
ExactMatch(String),
RegexMatch(Regex)
}
// default case -- assume exact match when a string is given
impl From<String> for LinksTagQuery {
fn from(match: String) -> Self {
LinksTagQuery::ExactMatch(match)
}
}
// this is where you get to be explicit about regexes
impl From<Regex> for LinksTagQuery
fn from(match: Regex) -> Self {
LinksTagQuery::RegexMatch(match)
}
} I don't know if that It also opens the door to different query types in the future, although that's getting into YAGNI territory. |
Oh, also, do we want to be able to do regex matching on |
Oh yeah it is for both. The PR name just refers to the tag. I think the enum approach is a great way to go. I think I will map them all into stringified regex immediately in the HDK and then it is uniform after that. |
oh, well if you do that, then I guess there isn't really any reason for |
So I'm thinking we have some cleverly named enums so it looks like:
|
@willemolding this is looking beautiful. I'm all for DSLs, and Rust enums appear to make it super easy. Should be trivial to then implement |
Ok I was able to implement it almost exactly as we specified. The only thing is type aliases for enums are experimental apparently so I just have one type for both the type and tag fields. This has also simplified some of the underlying implementation significantly. |
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.
oh crap! just realised I started a review and left it open. I didn't want to make this change directly, on account of not knowing much about Rust yet 😬
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.
Looks good to me!
Looks good to me too. I'll expand on how breaking the change is in the Dev Pulse -- logically it hasn't changed (much) but the addition of the new enum will require people to start using it.
Never underestimate the power of a third-party developer to break your things ;) |
PR summary
get_links now accepts an enum for the tag and type which has the following form:
This makes the link calls nice and self descriptive e.g.
This has also made the implementation significantly simpler as all of the variants are converted to their regex form in the HDK before being ingested by core. Previously the Option type was kept and mapped to a reserved char (*) for transporting over the network.
Also adds an apps spec test.
changelog
Please check one of the following, relating to the CHANGELOG-UNRELEASED.md
- summary of change [PR#1234](https://github.com/holochain/holochain-rust/pull/1234)