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

Extract characteristics for Logicals #250

Merged
merged 4 commits into from
Jan 16, 2024

Conversation

mint-thompson
Copy link
Collaborator

Fixes #238 and completes task CIMPL-1167.

Logical models use special extensions to set their type characteristics. Check for these extensions when extracting keywords. Do not create caret rules for these extensions.

Logical models use special extensions to set their type characteristics.
Check for these extensions when extracting keywords. Do not create caret
rules for these extensions.
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.

Everything that's here looks good, but if you run it on the CLI against a logical JSON w/ characteristics, you don't get any of the characteristics in the output FSH. It looks like the Logical class's toFSH() method needs to be updated in SUSHI. Foiled again by toFSH()!

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 really good to me. I tested it out with the branch in SUSHI that updates the toFSH function and the Characteristics are there and look good.

One small question I had -- does it make sense to include in either or both of the tests that extensions that are not the characteristics extensions are not included in the characteristics keyword and still have regular rules created?

cmoesel
cmoesel previously approved these changes Jan 10, 2024
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 looks good! I like Julia's idea to add a non-context extension to the test JSON to prove it still handles those correctly, but I also recognize that we have other tests that handle normal extensions (and they still pass), so I'm willing to accept this without that change.

@jafeltra
Copy link
Collaborator

but I also recognize that we have other tests that handle normal extensions (and they still pass)

I'm guessing we have test cases somewhere that handle extensions on Structure Definitions generically (although I haven't pinpointed exactly which test case covers this), but I don't think we have any tests that test if a Logical has non-characteristics extensions. We only have test cases that have extensions that are all characteristics. Maybe this is testing more than we need to still, but wanted to ask specifically about that. Or maybe I'm just missing something and we have already covered that through something else.

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! Thanks for updating those tests.

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.

It's good! Thanks!

@mint-thompson mint-thompson merged commit da09f9b into master Jan 16, 2024
14 checks passed
@mint-thompson mint-thompson deleted the cimpl-1167-characteristics-keyword branch January 16, 2024 21:01
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.

Support FSH 3.0 Characteristics keyword
3 participants