-
Notifications
You must be signed in to change notification settings - Fork 11
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
Switch remaining R5 Turtle RDF generation to TurtleParser implementation #148
Conversation
@tmprd could you please update this to the latest base branch, and then let's see if we can get @grahamegrieve 's attention to merge it into main. |
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.
It looks like this is no longer producing this file: baseFileName + ".ttl"
It does look like the parser has been switched, but was it intended to still output that file?
Thanks for your review @dotasek Expanding the changes shows where this file is produced now: https://github.com/HL7/kindling/pull/148/files#diff-0b45ecac1a10eb5075600a7cb0da87b7531c1c2795f7d2519db5a50e1d440a42R4495-R4506 I double checked the turtle files that were generated from this, and it only outputs files for R5. Is that expected? That is what I intended. It's not clear to me how different versions of FHIR are published as part of this process, and whether its possible to update older versions from this code. |
I think @ericprud figured out how versions are handled. Eric, can you answer this? |
If the Q is how do I handle version selection in APIs, I've been making an API call distinction between |
Will kindling/src/main/java/org/hl7/fhir/tools/publisher/Publisher.java Lines 3684 to 3692 in 78f6a16
If Publisher.java is to be used for R4 or below, then I can add a condition to ensure that this RDF generator is for R5 and above. If not, you can merge this. Currently, this RDF generator is used for any version other than R4B, which is logic used by serializeResource() :kindling/src/main/java/org/hl7/fhir/tools/publisher/Publisher.java Lines 2086 to 2087 in 78f6a16
serializeResource() or resource2Json(), but I just thought of this as an extra precaution.
|
@tmprd kindling is normally only used to generate the version of FHIR currently in development (at this juncture, R6). There was a special case of this with R5 and R4B being developed in parallel, but this was an exception. @grahamegrieve is this correct, or am I missing some details regarding publishing existing versions of the spec? |
This does use the generator, but the code you're pointing to doesn't create the necessary file. To replicate this, run kindling to publish FHIR. Look in the publish folder for this document:
Open the document in a web browser. There is a link that says I put a breakpoint in the master branch of kindling to verify, and this happens for 1460 files. All that needs to be done is for that returned string to be written to the appropriate file. The original code has a FileOutputStream already created in that place. You would just have to use PrintWriter or something to actually write out to it. |
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.
Ensure that output of resource2Ttl is output to appropriate file.
I would also rename that method to convertResourceToTtl.
Thank you! Actually I noticed it has to be written using kindling/src/main/java/org/hl7/fhir/tools/publisher/Publisher.java Lines 5384 to 5385 in 7b19236
This uses org.hl7.fhir.r5.elementmodel.TurtleParser instead of org.hl7.fhir.r4.formats.RdfParser.java. |
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.
The code changes now appear to generate the expected files.
Before signing off on this, I'd like someone to review the differences between the previous ttl generation results, and the ones from this PR. I ran the publish process on both, and packaged them in the attached zip.
Per the above request, I'm looking at the diffs between the old and new, to verify that they all look correct. To help, I've uploaded a diff of only the old and new fhir.ttl ontology file here: I will post again here after I've finished my comparison. |
Fixes:
Issues:
|
Thanks for checking this Eric! For example, the comment characters seem to be added in the org.hl7.fhir.utilities.turtle.Turtle.commitSection() method. I added an issue for this: w3c/hcls-fhir-rdf#150 |
@tmprd , thank you so much for your work on this, and sorry it took so long to get the results better vetted. (Thanks @ericprud for that!) I know I said I would give up on further vetting them if I had not gotten to it earlier, but I just couldn't bring myself to do that. I kept thinking that I'd find a way. And thankfully, @tmprd came to the rescue with a better vetting approach. Thanks all! |
@dotasek I believe we have adequately checked the diffs and verified that this PR is correct to merge. Can you go ahead and merge it, or is there anything else still needed? Github says that it still has "Changes requested", but I am assuming that it is referring to your request for us to check the diffs, which is now done. |
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.
After a run of this, I can see that no ttl files are missing or added. As the diff was approved, this looks good to me.
Thank you! |
Fixes these issues and also missing type annotations on profiles (ex. https://www.hl7.org/fhir/actordefinition.profile.ttl.html)
This simply switches some of the example generators to the R5+ TurtleParser instead of the RdfParser used before R5.