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

added PersonName support to Value #262

Merged
merged 14 commits into from
Jul 14, 2022
Merged

added PersonName support to Value #262

merged 14 commits into from
Jul 14, 2022

Conversation

jmlaka
Copy link
Contributor

@jmlaka jmlaka commented Jul 3, 2022

Hello Eduardo,
I don't know if this would be useful but I needed to handle Dicom PN.
It only uses borrowed values and family, given names are not optional but mandatory.

Removed one typo from a previous Doc test

Copy link
Owner

@Enet4 Enet4 left a comment

Choose a reason for hiding this comment

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

Hello @jmlaka! Thank you for sending another contribution in.

A person name module can indeed be helpful for systems working such names semantically. More than once have I received cases in which the system didn't bother encoding each component properly.

However, it is important for the baseline implementation to be more flexible so as to avoid issues caused by less ordinary names that could hinder the process for bystanders. I wouldn't require ideographic/phonetic character groups to be supported in an initial phase, but I would not want the given and family names to be mandatory, especially when the standard imposes that any of them can be an empty string.

I invite you to continue working on this module so that it accepts person names in which any of the components may be empty. It might also be more usable to employ the builder pattern to construct a person name from scratch. Feel free to follow up if you have any questions or could use some guidance.

Other minor concerns inline with the code.

core/src/value/person_name.rs Outdated Show resolved Hide resolved
core/src/value/person_name.rs Outdated Show resolved Hide resolved
core/src/value/person_name.rs Outdated Show resolved Hide resolved
core/src/value/person_name.rs Outdated Show resolved Hide resolved
core/src/value/person_name.rs Outdated Show resolved Hide resolved
@Enet4 Enet4 added the A-lib Area: library label Jul 3, 2022
@jmlaka
Copy link
Contributor Author

jmlaka commented Jul 5, 2022

Please see

@Enet4
Copy link
Owner

Enet4 commented Jul 12, 2022

I took the liberty of doing some final touches on this pull request. A summary:

  • extend and adjust documentation and tests (to also include doctests and cover the person name builder)
  • make PersonNameBuilder non-consuming
  • add From<PersonNameBuilder> and From<&mut PersonNameBuilder> impls to PersonName
  • rename PersonName::from_slice to PersonName::from_str
  • rename DicomValue::as_person_name to DicomValue::to_person_name for consistency with the rest of the API

In any case, the module seems robust and well constructed. @jmlaka I am likely to merge this soon, but feel free to look over the extra commits in case you have any concerns or questions.

@jmlaka
Copy link
Contributor Author

jmlaka commented Jul 13, 2022

Thanks for your insights @Enet4 ! Everything is fine with me.

Copy link
Owner

@Enet4 Enet4 left a comment

Choose a reason for hiding this comment

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

Thank you!

@Enet4 Enet4 merged commit c84ffcf into Enet4:master Jul 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lib Area: library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants