Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fixup DicomAnonymiser CTP implementation #2053

Merged
merged 56 commits into from
Jan 8, 2025
Merged

Conversation

rkm
Copy link
Member

@rkm rkm commented Dec 17, 2024

Proposed Changes

This PR finishes the wrapper for ctp-anon-cli and adds tests.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New Feature (non-breaking change which adds functionality)
  • Breaking Change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation-Only Update (if none of the other choices apply)
    • In this case, ensure that the message of the head commit from the source branch is prefixed with [skip ci]

Checklist

By opening this PR, I confirm that I have:

  • Reviewed the contributing guidelines for this repository
  • Ensured that the PR branch is in sync with the target branch (i.e. it is automatically merge-able)
  • Updated any relevant API documentation
  • Created or updated any tests if relevant
  • Created a news file
    • NOTE: This must include any changes to any of the following files: default.yaml, any of the RabbitMQ server configurations, GlobalOptions.cs
  • Listed myself in the CONTRIBUTORS file 🚀
  • Requested a review by the @SMI/Reviewers team

Issues

@rkm
Copy link
Member Author

rkm commented Dec 18, 2024

CC @jas88 feel free to pick this up if you have time

@rkm rkm force-pushed the feature/dicom-anonymiser-2 branch from e411aaf to 66b49dd Compare January 6, 2025 11:34
Copy link

codecov bot commented Jan 7, 2025

Codecov Report

Attention: Patch coverage is 73.11828% with 25 lines in your changes missing coverage. Please review.

Project coverage is 66.68%. Comparing base (3cc4bfb) to head (637f1cb).
Report is 56 commits behind head on main.

Files with missing lines Patch % Lines
...es/DicomAnonymiser/Anonymisers/SmiCtpAnonymiser.cs 76.36% 8 Missing and 5 partials ⚠️
...s/DicomAnonymiser/Anonymisers/DefaultAnonymiser.cs 0.00% 9 Missing ⚠️
...services/DicomAnonymiser/Helpers/ProcessWrapper.cs 82.35% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2053      +/-   ##
==========================================
+ Coverage   66.03%   66.68%   +0.64%     
==========================================
  Files         182      184       +2     
  Lines        6519     6535      +16     
  Branches      958      959       +1     
==========================================
+ Hits         4305     4358      +53     
+ Misses       1920     1877      -43     
- Partials      294      300       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rkm
Copy link
Member Author

rkm commented Jan 7, 2025

@jas88 any chance you could look at the failing windows tests? I think I'm missing something obvious!

@rkm rkm marked this pull request as ready for review January 8, 2025 10:15
@rkm rkm requested a review from jas88 January 8, 2025 15:15
@rkm
Copy link
Member Author

rkm commented Jan 8, 2025

Thanks @jas88! Are you happy for this to be merged now?

Copy link
Contributor

@jas88 jas88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as long as the retained event handler is intentional.

@rkm rkm merged commit 32d800c into main Jan 8, 2025
19 checks passed
@rkm rkm deleted the feature/dicom-anonymiser-2 branch January 8, 2025 16:16
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants