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

bugfix resource ID type #48

Merged
merged 1 commit into from
Jan 24, 2021
Merged

Conversation

ItayGoren
Copy link
Contributor

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

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
Copy link
Contributor Author

@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
Copy link
Owner

nazrulworld commented Jan 18, 2021

@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
Copy link
Owner

nazrulworld commented Jan 18, 2021

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
Copy link
Contributor Author

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

@nazrulworld
Copy link
Owner

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
Copy link
Contributor Author

i can see that they already has Id and not String

@nazrulworld
Copy link
Owner

nazrulworld commented Jan 18, 2021

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
Copy link
Contributor Author

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

@nazrulworld
Copy link
Owner

LGTM

@nazrulworld nazrulworld merged commit 00edad3 into nazrulworld:master Jan 24, 2021
@ItayGoren
Copy link
Contributor Author

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

@nazrulworld
Copy link
Owner

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

@nazrulworld
Copy link
Owner

Here we go 3ffb184
I Will update the readme, now we keep the Id type's value maximum of 64 but possible to extends!

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