-
Notifications
You must be signed in to change notification settings - Fork 4
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
Error msg for missing 'resourceType' property #157
Conversation
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.
Thanks for making this contribution @cbhutad! We really appreciate it! I think there may be some opportunities to make it even better. Upon review, we found that as long as one JSON object on one JSON editor had a resourceType
, the error message wouldn't get logged. Since it is possible to have multiple JSON objects processed at once using the "Convert to JSON" button, we actually want to log an error for each of the JSON objects that we find without a resourceType
. For example, if we have three JSON objects in three different JSON editors and one of them has a resourceType
and two do not have resourceType
, we want to log two error messages indicating the two JSON objects missing resourceType
.
To do this, I'd actually suggest making this change in the runGoFSH
function in FSHHelpers.js
. In that function, we parse out each resource that is being processed. Once we do that, we can check if the resourceType
property is available on that particular JSON object.
Because there could be multiple JSON objects that are present, we also would want to indicate which JSON object is missing resourceType
in the logged message. In the runGoFSH
function, we also create a default location
variable. I'd suggest an error message along the lines of this since id
may not always be available:
logger.error(`FHIR JSON ${resource.id ?? location} is missing the required "resourceType" property`);
Do these suggestions sound reasonable to you? If so, would you be willing to update this PR?
Hello Julia, I have tried to implement the changes you requested please check the below screenshots for the same. Here example-01 is valid JSON, example-02 is JSON with missing resourceType property and id as 'example-02' and Untitled one is JSON with both resourceType and id properties missing. Please let me know if this is desired result. |
Hi @cbhutad - Based on that screenshot, that looks exactly like what I had been thinking. Thanks for trying it out! Feel free to push up the changes whenever you're ready. |
Thanks julie for the confirmation. Just pushed the PR for the same. |
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.
Thanks for making the updates! This is exactly what I had in mind, and I think it will help users understand exactly where their error is a little more easily.
Once one other member of our team is able to review and approve this PR, we will merge and deploy it. Thanks again for contributing!
No description provided.