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

Allow MINUTE precision for datetimes. Closes #604 #655

Merged
merged 1 commit into from Jun 8, 2017

Conversation

ohr
Copy link
Collaborator

@ohr ohr commented May 26, 2017

No description provided.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 76.731% when pulling aafde2e on ohr:master into ba40f44 on jamesagnew:master.

jamesagnew added a commit that referenced this pull request Jun 8, 2017
@jamesagnew
Copy link
Collaborator

Looks awesome! Merging it now.

I've also added you as a committer to the project, which only seems to make sense since you're probably the longest standing HAPI contributor who is still active there is. :)

@jamesagnew jamesagnew merged commit 1b557b0 into hapifhir:master Jun 8, 2017
@grahamegrieve
Copy link
Collaborator

grahamegrieve commented Jun 8, 2017 via email

@jamesagnew
Copy link
Collaborator

Aw man, you're right. Why does HAPI have an enum value for that level of precision even? I think that probably dates back to the 0.1 release.

@jamesagnew
Copy link
Collaborator

Ok this has been reverted. There are some good fixes in this PR though.

@ohr do you have a use case for parsing at this precision? Presumably you should be adding :00 to the value you're putting in there.. I'm thinking we should get rid of the enum value entirely just to be safe.

Technically we could also always allow this to parse, but notify the IParserErrorHandler about it so that the calling code has the ability to configure how strict they want to be. I am always verrry reluctant to be lenient around date/time issues though. They have caused so many big problems in the V2 codebase over the years, including some pretty serious near-misses I know of in clinical settings.

@CarthageKing
Copy link

Looking at the original bug filing, I think what this PR was trying to fix was to allow minute precision when using date/time search parameters.

The STU3 definitions have an example wherein the date param is specified up to the MINUTE precision: http://hl7.org/fhir/STU3/search.html#date

[parameter]=lt2013-01-14T10:00
2013-01-14 matches, because it includes the part of 14-Jan 2013 before 10am
[parameter]=gt2013-01-14T10:00
2013-01-14 matches, because it includes the part of 14-Jan 2013 after 10am

Also this note from the same link:

Note: Time can consist of hours and minutes with no seconds, unlike the XML Schema dateTime type.

@jamesagnew
Copy link
Collaborator

Ahhh this is an interesting distinction.

Perhaps the right thing to do here is to apply this PR in BaseDateTimeType/Dt, but block the DateTime type from allowing that level of precision.

jamesagnew added a commit that referenced this pull request Jun 9, 2017
jamesagnew added a commit that referenced this pull request Jun 9, 2017
@jamesagnew
Copy link
Collaborator

Ok, I've created a new PR #672 that incorporates this fix, but only allows it for search parameters, not for datatypes. Will merge once the CI verifies that we're good.

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.

None yet

5 participants