Skip to content
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

Warn when temporal primitive is not formatted correctly #256

Merged
merged 5 commits into from
Mar 29, 2024

Conversation

mint-thompson
Copy link
Collaborator

Completes task CIMPL-1241. See also FSHOnline #152.

The FHIR types dateTime, date, time, and instant all have required formats. If a value of one of these types does not match the format, emit a warning. The rule to set this value is still created.

Something I'm thinking about for this is the usefulness of the warning message. Right now, it has the rule path, but no indication of what entity has the incorrectly formatted value. I could send along the entity name to getFshValue to use in the message, since by the time the value is returned, the information about the FHIR type is gone. Please let me know what you think about this warning message.

The FHIR types dateTime, date, time, and instant all have required
formats. If a value of one of these types does not match the format,
emit a warning. The rule to set this value is still created.
Copy link
Member

@cmoesel cmoesel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works well. Thank you!

In regard to your question, I think we should report out the name of the entity for which we found the problem; otherwise it's more difficult for the user to find the issue.

In the extractor, we already have other cases where we do this by passing in the SD with the ED. In getFSHValue, it looks like you'll need to add an additional argument (as you noted).

Copy link
Collaborator

@jafeltra jafeltra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a little bit of a bummer that we need the set of warnings in 3 different places, but that isn't really because of your branch, so oh well I guess.

I left a few questions in line, let me know if you want to chat about any of them.

src/extractor/CaretValueRuleExtractor.ts Outdated Show resolved Hide resolved
test/extractor/CaretValueRuleExtractor.test.ts Outdated Show resolved Hide resolved
src/processor/InstanceProcessor.ts Outdated Show resolved Hide resolved
Warning message for concept caret rules includes the concept path.
Warning message for values on Instances uses the target id instead of
name.
Copy link
Collaborator

@jafeltra jafeltra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just had a couple more questions.

src/processor/InstanceProcessor.ts Outdated Show resolved Hide resolved
test/extractor/CaretValueRuleExtractor.test.ts Outdated Show resolved Hide resolved
src/extractor/CaretValueRuleExtractor.ts Show resolved Hide resolved
Update test for caret rule on profile to more closely resemble realistic
output.
Copy link
Collaborator

@jafeltra jafeltra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me! Thanks for making those updates.

Copy link
Member

@cmoesel cmoesel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Mint! I bet this ended up hitting a lot more lines of code than you were expecting! Those pesky error messages!

@mint-thompson mint-thompson merged commit 52914b3 into master Mar 29, 2024
14 checks passed
@mint-thompson mint-thompson deleted the cimpl-1241-date-time-format branch March 29, 2024 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants