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

[toimage] basic bulk conversion #518

Merged
merged 4 commits into from
Jun 4, 2024
Merged

Conversation

PierreBou91
Copy link
Contributor

Hi,

I worked on this a few month ago (#397) but I lacked time to finish it. Today I wanted to finish it and somehow messed my merge so I closed the other PR and reopened this clean one from main/master.

The basic functionalities of bulk conversion work fine while retaining the previous behavior.

The user can pass a single file, a folder or several files.
If a single file is passed, the user can specify an output file name with the extension.
If there are multiple files (or several files collected in the folder) the user can optionally pass an output directory where the images will be stored. The user can also optionally pass a target extension for the images.
If a folder is passed, it can be searched recursively for dicom files.

The function convert_single_file() is a bit clumsy because we wanted to retain the previous behavior of being able to rename the output file when passing a single file.

I could add some other functionalities but I wanted to check with you first if it was worth doing, notably:

  • Handling name collisions
  • Giving the possibility to convert every frames of a multiframe dicom (this would mean changing the current frame_number behavior)

Please tell me if this is what you had in mind in #343 and what should be modified to be able to merge.

Thanks

@Enet4 Enet4 added enhancement A-tool Area: tooling C-toimage Crate: dicom-toimage labels May 16, 2024
@Enet4 Enet4 self-requested a review May 16, 2024 08:35
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 reviving this PR. I found one issue, but it should be ready after the fix.

toimage/src/main.rs Outdated Show resolved Hide resolved
@PierreBou91
Copy link
Contributor Author

Hello,

Thanks for taking the time to review and for your feedback.

Nice catch indeed, however I'm confused because I have extensionless files in my test set and it doesn't panic while it should (at least I thought unwrapping a None value was panicking).

In any case, your fix works perfectly fine so I'll commit it without .as_deref() as clippy suggested:

warning: derefed type is same as origin
   --> toimage/src/main.rs:251:8
    |
251 |     if output.extension().as_deref() != Some("dcm".as_ref()) && !output_is_set {
    |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `output.extension()`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_option_as_deref
    = note: `#[warn(clippy::needless_option_as_deref)]` on by default
    ```

@PierreBou91 PierreBou91 requested a review from Enet4 May 25, 2024 17:20
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.

I believe we're ready to bring this in. The proposed functionality enables bulk conversions without affecting CLI usage for converting files one at a time.

Thanks again!

@Enet4 Enet4 merged commit 536168b into Enet4:master Jun 4, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tool Area: tooling C-toimage Crate: dicom-toimage enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants