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

ENH: Add lookup and putting of private elements #508

Merged
merged 11 commits into from
Apr 25, 2024

Conversation

naterichman
Copy link
Contributor

Fairly simple lookup and adding of private tags with creator for InMemDicomObject as briefly discussed in #431

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.

Hey @naterichman, I made a quick code review, but it seems the branch also needs a rebase.

object/src/mem.rs Outdated Show resolved Hide resolved
object/src/lib.rs Outdated Show resolved Hide resolved
@naterichman
Copy link
Contributor Author

I’ll rebase. I could have sworn I checkout from the master branch. I’m still getting used to contributing to a repo that’s not my own on GitHub (use gitlab for work)

Anyway I was thinking, regarding your comment about the attribute selector API on #431 . Do you want to keep methods like this and add supporting methods for the attribute selector API or would you be more interested in merging them into one single API for private tag access?

I was thinking we could expand the AttributeSelector enum to include a PrivateGroup and PrivateElement definition and these methods would take the corresponding selectors instead of group and element separately. LMK your thoughts

@Enet4
Copy link
Owner

Enet4 commented Apr 22, 2024

I’ll rebase. I could have sworn I checkout from the master branch. I’m still getting used to contributing to a repo that’s not my own on GitHub (use gitlab for work)

It's OK, it's easy to miss the fact that one has to do both

  1. fetch the latest contributions from the upstream remote: git fetch origin;
  2. rebase against it in specific: git rebase origin/master instead of git rebase master unless the local master branch has also been updated.

Anyway I was thinking, regarding your comment about the attribute selector API on #431 . Do you want to keep methods like this and add supporting methods for the attribute selector API or would you be more interested in merging them into one single API for private tag access?

We can keep them separate. Each API usually stands on its own, meaning that one can do most of the things without the operations API. In other words, we can start with what we have in this PR, which is provide these methods to the in-mem DICOM object impl.

I was thinking we could expand the AttributeSelector enum to include a PrivateGroup and PrivateElement definition and these methods would take the corresponding selectors instead of group and element separately. LMK your thoughts

I'm not entirely sure how that would look like, but I suggest we defer to a separate PR. If #508 is cleaned up and made ready within the next day or two, it might just be ready in time for the upcoming major release.

@Enet4 Enet4 added A-lib Area: library C-object Crate: dicom-object enhancement labels Apr 22, 2024
* Format object/src/mem.rs and object/src/lib.rs
* Change from u16 to u8, removing one error type
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.

One more review iteration, mostly to emphasize that new parts of the public API should be documented, and to take care of clippy lints warnings that may emerge.

object/src/mem.rs Outdated Show resolved Hide resolved
object/src/mem.rs Outdated Show resolved Hide resolved
object/src/mem.rs Outdated Show resolved Hide resolved
object/src/mem.rs Outdated Show resolved Hide resolved
object/src/lib.rs Outdated Show resolved Hide resolved
object/src/mem.rs Outdated Show resolved Hide resolved
object/src/mem.rs Show resolved Hide resolved
@naterichman
Copy link
Contributor Author

Most of that is a big il’ facepalm. I’ll get this done today!

naterichman and others added 3 commits April 23, 2024 09:55
Co-authored-by: Eduardo Pinho <enet4mikeenet@gmail.com>
* Add docs for new public functions
* Make the PrivateElementError enum represent tags as hex
* Add some tests for error representation
object/src/mem.rs Outdated Show resolved Hide resolved
naterichman and others added 3 commits April 23, 2024 11:24
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.

Just added a few minor tweaks to the documentation in the last few commits.

The method signatures are good and the tests seem to cover the intended use cases well. Future work will hopefully tackle a more generalised awareness of private elements in the rest of the ecosystem, but it's great to have this PR as the first step towards it. Much appreciated! 👍

@Enet4 Enet4 merged commit 5908353 into Enet4:master Apr 25, 2024
4 checks passed
@Enet4 Enet4 linked an issue May 24, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lib Area: library C-object Crate: dicom-object enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding private data elements (with creator)
2 participants