Skip to content

Fi 4088 description update#52

Closed
rpassas wants to merge 10 commits into
mainfrom
FI-4088-description-update
Closed

Fi 4088 description update#52
rpassas wants to merge 10 commits into
mainfrom
FI-4088-description-update

Conversation

@rpassas

@rpassas rpassas commented Jun 13, 2025

Copy link
Copy Markdown
Contributor

Summary

Used LLM to extract standard practices/formats from existing descriptions to generate an 'guidance' files. These are partly manually edited md files to include constraints (ig reference, don’t touch generated files if there is a template, etc.). Then prompted an LLM to update metadata and descriptions group by group using the md files as guidelines.

@rpassas rpassas requested a review from arscan June 13, 2025 15:43
Comment thread lib/ips/allergy_intolerance.rb Outdated

test do
title 'Server returns correct AllergyIntolerance resource from the AllergyIntolerance read interaction'
title 'Server supports reading AllergyIntolerance resources'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is wrong, and is an interesting case where the LLM is missing a bit of the (important) nuance here. This definitely isn't checking to see the server supports 'reading' AllergyIntolerance resources at all. It is checking that is supports the 'read' interaction, and 'read' (maybe confusingly in this situation) is a verb from the client perspective. I think this is something we might have to explicitely guide the LLM to make sure it gets right (not sure the best way of doing it, but i could see this being a persistent problem if it isn't give this context).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I didn't fully appreciate the difference between reading and using a read interaction here. Also interesting that the title is formatted the original way in the LLM-generated template, but takes this form for the tests.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These guidance files don't need to be shipped with the ruby gem that contain the tests, as these files just help us create and maintain the tests and aren't used at 'runtime'. So, they should go in a docs/ folder at the top level.

Comment thread ips_test_kit.gemspec Outdated
spec.homepage = 'https://github.com/inferno-framework/ips-test-kit'
spec.license = 'Apache-2.0'
spec.add_runtime_dependency 'inferno_core', '>= 0.6.7'
spec.add_runtime_dependency 'inferno_core', '>= 0.6.11'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is there a specific feature we are now relying on that reuqires us to force all users of this gem to upgrade inferno core past 0.6.11. (ok if yes, but i though we were sticking with updating titles, descriptons, etc here).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I can't remember a specific reason for needing to update core dependency - will revert.

Comment thread lib/ips/metadata.rb Outdated
Comment on lines +17 to +36
# External Links
links [
{
label: 'Implementation Guide',
url: 'https://hl7.org/fhir/uv/ips/STU1.1/'
},
{
label: 'Source Code',
url: 'https://github.com/inferno-framework/ips-test-kit'
},
{
label: 'Issues',
url: 'https://github.com/inferno-framework/ips-test-kit/issues'
},
{
label: 'Inferno Framework',
url: 'https://inferno-framework.github.io/inferno-core/'
}
]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This isn't needed at the test kit metadata level (which, yeah, it probably should but for historic reasons we have it at the test suite level). And in fact, this breaks inferno so that it doesn't load. So you have to delete it.

Comment thread lib/ips/composition.rb Outdated

run do
assert_valid_resource(profile_url: 'http://hl7.org/fhir/uv/ips/StructureDefinition/Composition-uv-ips')
assert_valid_resource(profile_url: 'http://hl7.org/fhir/uv/ips/STU1.1/StructureDefinition/Composition-uv-ips')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is wrong. canonical urls don't put versions in the path like this.

If we want to be specific about the version in here, technically we should be doing http://hl7.org/fhir/uv/ips/StructureDefinition/Composition-uv-ips|1.1.0 everywhere. We kinda get that for 'free' as long as v1.1.0 is the only one being loaded in the validator, but i suppose we should be explicit everywhere about that if our test descriptions say that we are specifically checking stu1.1.

i think for this, just removing the STU1.1 in the middle and not worrying about managing a version in here is the way to go.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Interesting that this only occurs for composition.

@arscan

arscan commented Jul 31, 2025

Copy link
Copy Markdown
Contributor

Superseded by another PR.

@arscan arscan closed this Jul 31, 2025
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.

2 participants