Differ functionality (TEDM2O-12) #292
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
| exit 1; \ | ||
| fi | ||
| @if [ -z "${MERGE_OUTPUT_FILE}" ]; then \ | ||
| cd rdf-differ-ws && PATH="${ABSOLUTE_MODEL2OWL_FOLDER}/jena/apache-jena/bin:$$PATH" bash ./bash/merge-owl-shacl.sh "../${MERGE_ONTOLOGY_FILE}" "../${MERGE_SHAPES_FILE}"; \ |
There was a problem hiding this comment.
The cleaner solution would be to define a Make variable for the executable at the top of the script and use it here (in the same way we do it for saxon (${SAXON}).
I mean something like MERGE_OWL_SHACL?=${MODEL2OWL_FOLDER}/....
Also, you can invoke the script directly as it has the bash executable defined: https://github.com/meaningfy-ws/rdf-differ-ws/blob/master/bash/merge-owl-shacl.sh#L1
| exit 1; \ | ||
| fi | ||
| @if [ -z "${MERGE_OUTPUT_FILE}" ]; then \ | ||
| cd rdf-differ-ws && PATH="${ABSOLUTE_MODEL2OWL_FOLDER}/jena/apache-jena/bin:$$PATH" bash ./bash/merge-owl-shacl.sh "../${MERGE_ONTOLOGY_FILE}" "../${MERGE_SHAPES_FILE}"; \ |
There was a problem hiding this comment.
If we're going to support the second case of missing MERGE_OUTPUT_FILE (although it won't be triggered unless the user explicitly set the empty value for the variable because we have the default value), then it's a good idea to list the generated file at the end.
| @echo "OUTDIR: ${RDF_DIFF_OUTDIR}" | ||
| @echo "AP: ${RDF_DIFF_AP}" | ||
| @echo "TEMPLATE: ${RDF_DIFF_TEMPLATE}" | ||
| @cd rdf-differ-ws && bash ./bash/rdf-differ.sh --old ../${RDF_DIFF_FILE1} --new ../${RDF_DIFF_FILE2} --output ../${RDF_DIFF_OUTDIR} --profile ${RDF_DIFF_AP} --template ${RDF_DIFF_TEMPLATE} |
There was a problem hiding this comment.
Re ../${RDF_DIFF_OUTDIR} and other occurrences: Please don't do this as it's obscure and error-prone. Use variables as-is.
| @echo "OUTDIR: ${RDF_DIFF_OUTDIR}" | ||
| @echo "AP: ${RDF_DIFF_AP}" | ||
| @echo "TEMPLATE: ${RDF_DIFF_TEMPLATE}" | ||
| @cd rdf-differ-ws && bash ./bash/rdf-differ.sh --old ../${RDF_DIFF_FILE1} --new ../${RDF_DIFF_FILE2} --output ../${RDF_DIFF_OUTDIR} --profile ${RDF_DIFF_AP} --template ${RDF_DIFF_TEMPLATE} |
There was a problem hiding this comment.
Re the executable: The same comment as in case of the merging script above.
| @cd rdf-differ-ws && bash ./bash/rdf-differ.sh --old ../${RDF_DIFF_FILE1} --new ../${RDF_DIFF_FILE2} --output ../${RDF_DIFF_OUTDIR} --profile ${RDF_DIFF_AP} --template ${RDF_DIFF_TEMPLATE} | ||
| @echo "🔎 First few lines of the diff report:" | ||
| @if [ -f "${RDF_DIFF_OUTDIR}/diff.${RDF_DIFF_TEMPLATE}" ]; then \ | ||
| head ${RDF_DIFF_OUTDIR}/diff.${RDF_DIFF_TEMPLATE}; \ |
There was a problem hiding this comment.
Do we need this? It looks like a debug print to me. You could conditionally print it, controlling it with a new DEBUG or VERBOSE Make variable.
| # RDF_DIFF_FILE2: Path to the second RDF file (default: test/diffing-files/ePO_core-4.2.0.ttl) | ||
| # RDF_DIFF_OUTDIR: Output directory for diff results (default: ${OUTPUT_FOLDER_PATH}, which is "output") | ||
| # RDF_DIFF_AP: Application profile (default: owl-core-en-only) | ||
| # RDF_DIFF_TEMPLATE: Output template format (default: html) |
There was a problem hiding this comment.
Let's spare the user searching and list the available options here: json, asciidoc, html
| # RDF_DIFF_OUTDIR: Output directory for diff results (default: ${OUTPUT_FOLDER_PATH}, which is "output") | ||
| # RDF_DIFF_AP: Application profile (default: owl-core-en-only) | ||
| # RDF_DIFF_TEMPLATE: Output template format (default: html) | ||
| run-rdf-diff: start-rdf-differ-services |
There was a problem hiding this comment.
I see the value in simplifying the actions required from the user, but starting and stopping the services is not suitable in our scenario (model2owl-boilerplate workflow) where we want to execute this command twice (for asciidoc and json). Several options are possible as a solution here, you can consider adding another overarching recipe aggregating the actions, but that one would be for the specific use case (which may not be a bad thing). Alternatively, user would need to execute the sequence of commands manually.
There was a problem hiding this comment.
Note that the full workflow is an expensive operation so it would be good to have a recipe to execute diffing once (diff) and then generation of reports (report) twice instead of calling the existing run-rdf-diff recipe twice. See more here: https://github.com/meaningfy-ws/rdf-differ-ws?tab=readme-ov-file#examples
No description provided.