feat: add hci converter #23
Conversation
|
@cmdoret Thanks a lot for your review and most of your point make a lot of sense. As a general rule, I think it would be good to not add commits on a PR that you review, since that makes it harder to continue to work on it. At least in case you add a commit you should also point that out in a comment. I just noticed this when I tried to push my changes and had to first put them on another branch in order to pull the changed upstream first. It is okay for now, just maybe for next time. |
|
@vancauwe , @cmdoret With these changes it does not make sense to have a synth converter and an hci converter anymore: I will make just one metadata-converter that takes an extra argument |
|
@sabinem indeed sorry about that, I will set up formatting and CI to at least notify and check that the PR is formatted before accepting merges (without creating commits). |
| cargo run --bin hci-converter "{{root_dir}}/{{input_file}}" "{{root_dir}}/{{output_file}}" {{args}} | ||
| run input_type input_file output_file *args: | ||
| cd "{{root_dir}}/src/converter" && \ | ||
| cargo run --bin converter "{{input_type}}" "{{root_dir}}/{{input_file}}" "{{root_dir}}/{{output_file}}" {{args}} |
There was a problem hiding this comment.
| cargo run --bin converter "{{input_type}}" "{{root_dir}}/{{input_file}}" "{{root_dir}}/{{output_file}}" {{args}} | |
| cargo run --bin converter "{{input_type}}" "{{input_file}}" "{{output_file}}" {{args}} |
suggestion: the command is already executed from the root dir, I don't think we need to prefix individual arguments. Removing those prefixes also allows to specify external files via absolute path.
There was a problem hiding this comment.
@cmdoret I tried this but it did not work for me.
The current command is like this:
run input_type input_file output_file *args:
cd "{{root_dir}}/src/converter" && \
cargo run --bin converter "{{input_type}}" "{{root_dir}}/{{input_file}}" "{{root_dir}}/{{output_file}}" {{args}}
It works from everywhere in the projects directory tree and you first go down to {{root_dir}}/src/converter and then it searches from there if you don't add {{root_dir}} again. So I leave it as it is now.
src/converter/src/main.rs
Outdated
There was a problem hiding this comment.
praise: the CLI is much better like this, I think this is a good design that we can keep building on.
src/converter/src/convert.rs
Outdated
| "jsonld" => { | ||
| graph_builder.serialize_to_jsonld().context("Failed to serialize to JSON-LD")? | ||
| } | ||
| _ => graph_builder.serialize_to_turtle().context("Failed to serialize to Turtle")?, |
There was a problem hiding this comment.
comment: I would expect "nt" to return ntriples, but it would silently return ttl.
nitpick: match "turtle" to the proper serializer and panic or return an error in the default case.
| macro_rules! ns_entries_direct { // For rdf and xsd | ||
| ($msg:expr, $($ns:ident),*) => { | ||
| vec![ | ||
| $( | ||
| (stringify!($ns), $ns.get("").expect(&$msg)), | ||
| )* | ||
| ] | ||
| }; | ||
| } | ||
|
|
There was a problem hiding this comment.
praise: smart solution! looks like a good use of macros 👍
|
@cmdoret I followed up on your suggestions. Please dev review again. |
the hci converter parses the hci file and ignores the campaign warpper but stores `hasCampaign` struct as `cat:Campaign`
* add terms to existing namespace for Campaign, Batch and Objective * add namespaces allohdf and allocom
* update batch: map all properties to graph * add Objective * complete properties for Campaign
Actions are optional on batches as for the hci parser there are no actions for batches
* provide example input data for HCI Parser
as the converter now converts both synth and hci metadata
That way it is detected in case the format is not recognized.
sabinem
force-pushed
the
feat/add-hci-converter
branch
from
b110af1 to
5f69d07
Compare
This PR adds the HCI converter together with tests:
An example for the expected HCI input data is provided at
examples/0-HCI.jsonThe commands for running the converters was changed:
just run-synth examples/1-Synth.json examples/1-Synth.ttljust run-hci examples/0-HCI.json examples/0-HCI.ttljust testNote: We are not checking enums yet and leave that to the Shacl Validation: such as
We only check them where they are of importance for the transformation such as to identify the action type in a batch. The validation of the values is currently left to the Shacl Validation, which makes the parsing a bit lighter and is okay from my perspective.
TODOs
typewas missing, but will be fixed soon.