Conversation
88b7876 to
fecf2bf
Compare
lfbrehm
left a comment
There was a problem hiding this comment.
I do not understand all of it, especially RDF semantic-related stuff, but did my best to provide a somewhat deep review. I also skipped tests and examples for the most part, because I trust that you ran the tests before.
| let base_rebuilt = base_parts.join("/"); | ||
| if base_rebuilt.is_empty() { | ||
| return rel_remaining.to_string(); | ||
| } |
There was a problem hiding this comment.
Does this mean, that if relative part is for example ../../../ but absolute only states /root/second-level that it returns ../ which should not be a valid solution? Shouldnt this result in an error?
There was a problem hiding this comment.
You are correct, haven't thought about it. Will be fixed in the next commit by throwing an error !
| || term.starts_with("../") | ||
| || term.starts_with('#') | ||
| || term.starts_with('/') | ||
| || term.contains('/') |
There was a problem hiding this comment.
Why is this a relative IRI instead of an absolute IRI? If it contains a / this can also be absolute, because you the (absolute) base is split in resolve_relative_iri.
There was a problem hiding this comment.
This occurs, after absolute iris have already been filtered, out, so the functionality is correct, but I will rename the function and add a check for absolute iris to prevent misuse !
src/ro_crate/rdf/context.rs
Outdated
| if best.is_none() || len > best.unwrap().2 { | ||
| best = Some((prefix, namespace, len)); | ||
| } |
There was a problem hiding this comment.
I see that this cannot fail, but wouldnt it be better to just rewrite this with match guards to avoid unwraps?
match best {
Some((p, n, l)) if len > l => ...
}
There was a problem hiding this comment.
Yes, even better is map_or:
if best.map_or(true, |(_, _, best_len)| len > best_len) {...
src/ro_crate/rdf/rdf_io.rs
Outdated
| if subject_str.contains("ro-crate-metadata.json") { | ||
| metadata_iri = Some(subject_str.to_string()); | ||
| break; | ||
| } |
There was a problem hiding this comment.
If you have multiple subcrates that are possibly defined with ./prefix/ro-crate-metadata.json, this would also match these subcrates, but you only want to find the root crate in this function.
There was a problem hiding this comment.
The ro-crate spec also states that the root metadata descriptor must have @id ro-crate-metadata.json or ro-crate-metadata.jsonld for legacy crates.
There was a problem hiding this comment.
This behavior is directly from the spec:
but yes it can match multiple results (and some results that are not the root crate). We should maybe check if the entity matched is a subcrate entity if yes skip it. Btw. contains("ro-crate-metadata.json") also matches: "ro-crate-metadata.jsonld"
src/ro_crate/rdf/convert.rs
Outdated
| /// Resolve relative IRIs against the provided base IRI. | ||
| /// This takes precedence over @base defined in the context. | ||
| WithBase(String), |
There was a problem hiding this comment.
Dont you want also the opposite where @base takes precendence and only if this cannot not be resolved you resolve to the specified one?
There was a problem hiding this comment.
You are correct, will be fixed.
src/ro_crate/rdf/rdf_io.rs
Outdated
| properties.entry(property_name).or_default().push(value); | ||
| } | ||
| } | ||
|
|
||
| // Convert Vec<EntityValue> to EntityValue (single or array) | ||
| let mut result = HashMap::new(); | ||
| for (key, values) in properties { | ||
| if values.len() == 1 { | ||
| result.insert(key, values.into_iter().next().unwrap()); | ||
| } else if values.len() > 1 { | ||
| result.insert(key, EntityValue::EntityVec(values)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Same as above: Why not do this when using the entry api of the HashMap to skip looping again over your results?
There was a problem hiding this comment.
The entry API will not really work here ?! I mean I want to iterate all elements from the list. But we could make it more neat using an iterator / filter_map statement.
| let compacted_id = if metadata_iri.contains("ro-crate-metadata.json") { | ||
| "ro-crate-metadata.json".to_string() | ||
| } else { | ||
| context.compact_iri(metadata_iri) | ||
| }; |
There was a problem hiding this comment.
This will probably also match subcrates right?
There was a problem hiding this comment.
Not a problem, this only occurs if the correct metadata descriptor has already been found, so this will not target subcrates !
| /// Internal implementation: Convert RDF triples to RoCrate using a ResolvedContext. | ||
| fn rdf_to_rocrate_with_context( | ||
| triples: Vec<Triple>, | ||
| context: ResolvedContext, | ||
| ) -> Result<RoCrate, RdfError> { |
There was a problem hiding this comment.
Why not merge this and the rdf_graph_to_ro_crate fns together, if you use the wrapper fn only anyway?
There was a problem hiding this comment.
There are two wrappers: rdf_graph_to_rocrate and rdf_to_rocrate both use this function (with different context)
src/ro_crate/rdf/rdf_io.rs
Outdated
| let base_iri = base | ||
| .map(|b| b.to_string()) | ||
| .or_else(|| infer_base_from_metadata(&metadata_iri)) | ||
| .unwrap_or_else(|| "http://example.org/".to_string()); |
There was a problem hiding this comment.
Is http://example.org really meant as a default fallback value or just a placeholder that never got replaced?
There was a problem hiding this comment.
Ups, yes this is leftover from a testing session -> fixed
| Self { | ||
| cache: HashMap::new(), | ||
| allow_remote: false, | ||
| client: reqwest::blocking::Client::builder() | ||
| .timeout(std::time::Duration::from_secs(30)) | ||
| .build() | ||
| .expect("Failed to create HTTP client"), | ||
| } |
There was a problem hiding this comment.
Why initialize a client when allow_remote is false?
There was a problem hiding this comment.
Fair point, will make it optional and initialize it only if allow_remote is set to true. Thinking about this allow_remote should omit the boolean so allow_remote should just set it to true !
microchange: Fix typo on "Research" !
|
will merge in separate PR into upstream! |
This feature adds a RDF to ROCrate interoperability module that allows to export ROCrates as RDF graph and import ROCrates from RDF graphs. All conversion is funneled through the
ROCratestruct.rdffeature flagFuture implications: The full ROCrate expansion to RDF triples can be used to improve semantic validation of the ROCrate.
Completes: https://github.com/arunaengine/project-orga/issues/26
This requires the changes from: #2 and should be rebased when this is merged.