search

Enet4 / dicom-rs

Rust implementation of the DICOM standard
https://dicom-rs.github.io
Apache License 2.0
416 stars 81 forks source link

ENH: Add lookup and putting of private elements #508

Closed naterichman closed 5 months ago

naterichman commented 5 months ago

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

naterichman commented 5 months ago

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 commented 5 months ago

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.

naterichman commented 5 months ago

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