fix: remove deprecated pydicom.dicomdir.DicomDir variable type option #130
Conversation
WalkthroughThe changes involve modifications to the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/imgtools/utils/dicomutils.py (2)
Line range hint
2-2: Remove unused Union importSince the function signature no longer uses a Union type, the
Unionimport from typing can be removed.-from typing import Dict, TypeVar, Union +from typing import Dict, TypeVar
62-62: Add function documentationConsider adding a docstring to document the function's purpose, parameters, and return value structure. This is particularly important after removing DicomDir support to guide users.
def all_modalities_metadata(dicom_data: pydicom.dataset.FileDataset) -> Dict[str, T]: + """Extract common metadata from a DICOM dataset. + + Args: + dicom_data (pydicom.dataset.FileDataset): The DICOM dataset to extract metadata from + + Returns: + Dict[str, T]: Dictionary containing metadata with keys for common DICOM attributes + including PixelSize when available + """ metadata = get_modality_metadata(dicom_data, 'ALL')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/imgtools/utils/dicomutils.py (1 hunks)
🧰 Additional context used
🔇 Additional comments (3)
src/imgtools/utils/dicomutils.py (3)
Line range hint
63-70: Implementation remains robust after type changeThe function implementation uses defensive attribute checks and standard DICOM attributes, making it compatible with the FileDataset-only type signature. The core logic remains sound.
62-62: Add BREAKING CHANGE note to commit messageThis change removes support for DicomDir objects which could affect downstream code. Consider updating the commit message to include a BREAKING CHANGE note:
fix: remove deprecated pydicom.dicomdir.DicomDir variable type option BREAKING CHANGE: all_modalities_metadata now only accepts pydicom.dataset.FileDataset objects. Support for pydicom.dicomdir.DicomDir has been removed.Let's check for potential downstream impacts:
62-62: Verify impact of removing DicomDir supportThe signature change to only accept
FileDatasetaligns with removing deprecated functionality. However, we should verify that this change doesn't break existing code.Let's verify all usage patterns of this function:
✅ Verification successful
Let me gather more context about the usage pattern by examining the calling function and its implementation.
Based on the gathered information, I can now generate the final response:
Signature change to
FileDatasetappears safeThe codebase analysis shows:
- No explicit
DicomDirusage or type hints found in the fileall_modalities_metadatais only called fromget_modality_metadatawhich doesn't enforce any specific DICOM type- The implementation works with standard DICOM metadata fields that are common to
FileDatasetThe rationale for this verification:
- No evidence of
DicomDirspecific usage in the codebase- The function is only used internally by
get_modality_metadatawhich processes standard DICOM metadata fields- The signature change aligns with removing deprecated functionality without breaking existing code patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any instances where DicomDir objects might be passed to all_modalities_metadata # Search for imports or usage of DicomDir echo "Checking for DicomDir usage patterns..." rg -l "DicomDir" # Search for direct calls to all_modalities_metadata echo "Checking function call patterns..." ast-grep --pattern 'all_modalities_metadata($$$)'Length of output: 327
Script:
#!/bin/bash # Check the implementation of get_modality_metadata and surrounding context echo "Checking get_modality_metadata implementation..." rg -A 10 -B 10 "def get_modality_metadata" src/imgtools/utils/dicomutils.py # Check if there are any DicomDir type hints or assertions in the file echo "Checking for DicomDir type hints..." rg "DicomDir" src/imgtools/utils/dicomutils.pyLength of output: 1042
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #130 +/- ##
=======================================
Coverage 45.69% 45.69%
=======================================
Files 30 30
Lines 2867 2867
=======================================
Hits 1310 1310
Misses 1557 1557 ☔ View full report in Codecov by Sentry. |
Inclusion of this variable type was causing an error when running latest version of med-imagetools.
Summary by CodeRabbit
New Features
FileDatasetinstances.Bug Fixes