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 functionality for mutating element values #471

Merged
merged 6 commits into from
Apr 13, 2024

Conversation

naterichman
Copy link
Contributor

  • Add value_at_mut, entry_at, entry_at_mut and update_value_at methods on InMemDicomObject struct.
  • Add supporting methods to dependent objects

Let me know if this is something you would like, I'm finding I need something like this for a downstream application. I realize I can use the update_value method, but its nice to be able to update_value with an AttributeSelector. If there is a way to do that and I'm missing it, I'm happy to just update the docs to make that clearer instead of this PR.

@naterichman naterichman marked this pull request as draft March 11, 2024 15:11
@naterichman
Copy link
Contributor Author

If this is something you'd like, I'll go ahead and polish by adding docs, tests, and cleaning up

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 for this proposal. Unfortunately, some methods of that nature were deliberately kept away from the object API until now because it opens room for inconsistencies in the object. More details inline.

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

Removed or switched to private the methods you mentioned would be dangerous. Ended up keeping exposed

  • update_value_at
  • entry_at

Also called out in the docs for each function the alternatives (i.e. apply and value_at)

@naterichman naterichman requested a review from Enet4 March 12, 2024 16:34
@naterichman naterichman marked this pull request as ready for review March 12, 2024 16:35
@Enet4 Enet4 added enhancement A-lib Area: library C-object Crate: dicom-object labels Mar 16, 2024
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 for the reiteration. Just some finishing touches and it should be ready.

object/src/mem.rs Outdated Show resolved Hide resolved
object/src/mem.rs Outdated Show resolved Hide resolved
@naterichman naterichman requested a review from Enet4 March 16, 2024 14:28
naterichman and others added 5 commits April 11, 2024 08:40
* Add `value_at_mut`, `entry_at`, `entry_at_mut` and `update_value_at`
  methods on `InMemDicomObject` struct.
* Add supporting methods to dependent objects
* Remove dangerous methods
* Add documentation for new methods that are staying
- remove duplicate value_at method
@Enet4 Enet4 force-pushed the update-value-mut-improv branch from 4ab3571 to 6978b2c Compare April 11, 2024 07:44
- needless borrow in `.get(&tag)`
- work around rust-analyzer bug in line 1023
@Enet4 Enet4 force-pushed the update-value-mut-improv branch from 41636bc to 7542ff5 Compare April 13, 2024 10:43
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.

OK, this is ready. Thank you @naterichman. 👍

@Enet4 Enet4 merged commit 84c72d1 into Enet4:master Apr 13, 2024
4 checks passed
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.

2 participants