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

switch the type of Element.id from id to string for R4B #160

Merged
merged 1 commit into from
Oct 24, 2024

Conversation

cybernop
Copy link
Contributor

According to the FHIR documentation Element.id should be of type string.

StructureDefinitions with type slicing will have entries in the differential and snapshot part in the format like Observation.category:VSCat. This is valid according to the specification. But before the element was of type id which caused validation errors when parsing StructureDefinitions like this.

@nazrulworld
Copy link
Owner

nazrulworld commented Jul 28, 2024

@cybernop
Copy link
Contributor Author

@cybernop Thanks a lot for your PR. Please have a look here https://github.com/nazrulworld/fhir.resources?tab=readme-ov-file#resourceid-aka-fhirtypesid-constraint-extensibility

Here is an example of patch https://github.com/nazrulworld/fhir.resources/blob/main/fhir/resources/core/patch/patch_r4b_test.py

@nazrulworld Thanks for the reply. But I do not fully understand what you are trying to tell me. Do you want to tell me error is on the user side or should should I add a test case? I do not want to to change the length of Id nor do I want to create a patch for something. I just wanted to fix an error in the data type definition.

In my opinion the type of Element.id in R4B is incorrect. In the definition it is of type string and all other version (DSTU2, STU3 and R5) use the correct data type for Element.id. Only in R4B id was used incorrectly which prevents using the library in my project because of invalid parsing errors.

@cybernop
Copy link
Contributor Author

I just want to stress that this is an issue of incorrect type and not the length of ID

@Horstage
Copy link

That's especially annoying since it's only the case for R4B, not for DSTU2, STU3 or R5.
The FHIR definition for Elements states, that Element.id is a string in all versions.

Copy link

@Horstage Horstage left a comment

Choose a reason for hiding this comment

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

Types have changed in f604fb1.
It is now StringType

@cybernop
Copy link
Contributor Author

Updated the type to use the new StringType

@cybernop
Copy link
Contributor Author

I want to stress how important this issue is, as in Germany we only use R4(B) at the moment and this issue prevents us to use the library for any StructureDefinition

@nazrulworld
Copy link
Owner

nazrulworld commented Oct 23, 2024

@cybernop This PR should be merged with the branch 7.X.X, right?

@Horstage
Copy link

I took the liberty to create a separate PR #165 for 7.x.x as it should be fixed in both 7.x.x and main, but the commits differ slightly.

@@ -43,7 +43,7 @@ class Element(fhirabstractmodel.FHIRAbstractModel):
},
)

id: fhirtypes.IdType | None = Field( # type: ignore
id: fhirtypes.StringType = Field(

Choose a reason for hiding this comment

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

My last comment was incomplete, sorry.
Comparing with fhir/resources/element.py, fhirtypes.StringType | None is the correct type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes you are right, I adjusted my PR

According to the FHIR documentation Element.id should be of type string. Before it was of type id which caused errors when parsing structure definitions with type scling what results the have `:` in their id. Which is allowed and supported for string but not for id.
@cybernop
Copy link
Contributor Author

cybernop commented Oct 24, 2024

@nazrulworld This change should be merged into main.

PR #165 by @Horstage covers my original modification compatible to 7.X.X and should be merged into that branch.

@nazrulworld nazrulworld merged commit cb5b4c8 into nazrulworld:main Oct 24, 2024
1 check was pending
@cybernop cybernop deleted the fix/element-id branch October 24, 2024 09:04
nazrulworld added a commit that referenced this pull request Oct 24, 2024
nazrulworld added a commit to nazrulworld/fhir-parser that referenced this pull request Oct 24, 2024
@nazrulworld
Copy link
Owner

This new release should have been your PR included https://pypi.org/project/fhir.resources/8.0.0b4/

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