search

nazrulworld / fhir.resources

FHIR Resources https://www.hl7.org/fhir/resourcelist.html
https://pypi.org/project/fhir.resources/
Other
372 stars 104 forks source link

bugfix resource ID type #48

Closed ItayGoren closed 3 years ago

ItayGoren commented 3 years ago

The ID type of the field id should be Id and not String as documented here: https://www.hl7.org/fhir/resource-definitions.html#Resource.id

ItayGoren commented 3 years ago

@nazrulworld a single test if failing because the resource ID is longer than 64 characters. how do you recommend handling it? is it a pre-downloaded test or we can edit it?

nazrulworld commented 3 years ago

@ItayGoren thanks 🙏 a lot for your pull request, I. All test resources are downloaded runtime, but the technically possible update that changes.

For this change (String to Id), I would like to wait and get full confirmation from FHIR community (You can see my post here Resource.id Data Type), I think technically it is possible to change the datatype of Resource.id by using an extension. Also, it would be breaking, into the existing system, if someone allows more than 64 characters.

nazrulworld commented 3 years ago

Update

Full discussion here https://chat.fhir.org/#narrow/stream/179166-implementers/topic/Resource.2Eid.20restrictions

I think we have two options right now.

  1. Let the user do that validation by using a custom validator [gives them a lot of flexibility, but some sure if problematic to communicate otherssystem]
  2. We restricted from here (as you did ) by making as Id DataType (my vote on this option)
ItayGoren commented 3 years ago

i think we should go with option 2 as the documentation says. otherwise automatic tools that works by the documentation would fail

nazrulworld commented 3 years ago

i think we should go with option 2 as the documentation says. otherwise automatic tools that works by the documentation would fail

Can you please add the same for STU3/resource.py and DSTU3/resource.py respectively?

ItayGoren commented 3 years ago

i can see that they already has Id and not String

nazrulworld commented 3 years ago

i can see that they already has Id and not String

That is interesting, have to check to specifications (JSON) files for R4.

Update

Problem is that https://www.hl7.org/fhir/R4/resource.profile.json.html here Resource.id type is String! https://chat.fhir.org/#narrow/stream/179166-implementers/topic/Resource.2Eid.20Data.20Type/near/223161848 We cannot do it manually. awaiting for their answer.

ItayGoren commented 3 years ago

nice work! now we wait, i guess it would take them some time...

nazrulworld commented 3 years ago

LGTM

ItayGoren commented 3 years ago

@nazrulworld i dont see they changed it... 😞

nazrulworld commented 3 years ago

But I have some idea over fhirtypes.Id, keep in touch update coming soon.

nazrulworld commented 3 years ago

Here we go https://github.com/nazrulworld/fhir.resources/commit/3ffb1848ae78cd042f9eafb8dec8c49ee9c4f526 I Will update the readme, now we keep the Id type's value maximum of 64 but possible to extends!