Skip to content

[toimage] basic bulk conversion#518

Merged
Enet4 merged 4 commits intoEnet4:masterfrom
PierreBou91:imp/toimage/bulk
Jun 4, 2024
Merged

[toimage] basic bulk conversion#518
Enet4 merged 4 commits intoEnet4:masterfrom
PierreBou91:imp/toimage/bulk

Conversation

@PierreBou91
Copy link
Copy Markdown
Contributor

@PierreBou91 PierreBou91 commented May 15, 2024

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
Enet4
Enet4 requested changes May 25, 2024
View reviewed changes
Copy link
Copy Markdown
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.

@PierreBou91
Copy link
Copy Markdown
Contributor Author

PierreBou91 commented May 25, 2024

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
Enet4
Enet4 approved these changes Jun 4, 2024
View reviewed changes
Copy link
Copy Markdown
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
@abroy77 abroy77 mentioned this pull request Nov 8, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Enet4 Enet4 Enet4 approved these changes

Assignees

No one assigned

Labels

A-tool Area: tooling C-toimage Crate: dicom-toimage enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@PierreBou91 @Enet4