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

Fix to handle date fields with empty values #190

Merged
merged 2 commits into from
Sep 21, 2021

Conversation

wetzelj
Copy link
Contributor

@wetzelj wetzelj commented Sep 21, 2021

Added check to prevent exception when JITTER action is performed on a field with a blank value.

Description

Related issues: #189

  • Added check to ensure a new jittered date value has a value before attempting to perform the replace of the field.
  • Modified test dicom file to enable the creation of a new unit test to validate this change.

Checklist

  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • My code follows the style guidelines of this project

Open questions

Questions that require more discussion or to be addressed in future development:

Added check to prevent exception when JITTER action is performed on a field with a blank value.
@@ -465,7 +465,8 @@ def _run_action(self, field, action, value=None):
if value is not None:
# Jitter the field by the supplied value
new_val = jitter_timestamp(field=field, value=value)
self.replace_field(field, new_val)
if new_val is not None:
Copy link
Member

Choose a reason for hiding this comment

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

So this would be okay as an empty string?

Copy link
Member

Choose a reason for hiding this comment

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

(or actually is it not possible to return that?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

get_timestamp() currently only returns None or a formatted timestamp, but, I'll look for both just in case. Doesn't hurt to be extra careful.

Checking new value for space in addition to none
@vsoch vsoch merged commit d839e8c into pydicom:master Sep 21, 2021
@vsoch
Copy link
Member

vsoch commented Sep 21, 2021

Thank you @wetzelj !

@wetzelj
Copy link
Contributor Author

wetzelj commented Sep 21, 2021

Thank you @vsoch! You're always so speedy getting these things merged in... I really appreciate it!

@wetzelj
Copy link
Contributor Author

wetzelj commented Sep 22, 2021

@vsoch... I'm irritated with myself. :( I updated the readme in this PR, but didn't update version.py. Do you want me to create a separate pull request for the version update or wait until the next PR since #186 is still open out there?

@vsoch
Copy link
Member

vsoch commented Sep 22, 2021

Oh don’t worry I updated it locally and just need to push the change!

@wetzelj wetzelj deleted the fix/date-jitter-space branch November 21, 2022 16:19
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