-
Notifications
You must be signed in to change notification settings - Fork 267
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
save_qgis.py
: Use single context manager for 4 files
#1310
Conversation
No need for the quadruple nesting
Reviewer's Guide by SourceryThis PR refactors the Sequence diagram for file handling in write_shape_filesequenceDiagram
participant F as write_shape_file
participant H as HDF5 Files
Note over F, H: Before: Nested context managers
F->>H: Open TimeSeries file
activate H
F->>H: Open Coherence file
F->>H: Open Velocity file
F->>H: Open Geometry file
F->>H: Process data
H-->>F: Close all files
deactivate H
Note over F, H: After: Single context manager
F->>H: Open all files simultaneously
activate H
F->>H: Process data
H-->>F: Close all files
deactivate H
Flow diagram showing file handling improvementflowchart LR
subgraph Before
A[Start] --> B[Open TimeSeries]
B --> C[Open Coherence]
C --> D[Open Velocity]
D --> E[Open Geometry]
E --> F[Process Data]
F --> G[Close Files]
G --> H[End]
end
subgraph After
I[Start] --> J[Open All Files]
J --> K[Process Data]
K --> L[Close Files]
L --> M[End]
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
PR summaryThis Pull Request refactors the Review Checklist
SuggestionConsider adding a test case to ensure that the refactored context manager behaves as expected, especially if there are any edge cases related to file handling that should be verified. This comment was generated by AI. Information provided may be incorrect. Current plan usage: 0% Have feedback or need help? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @scottstanie - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you @scottstanie!
Description of proposed changes
No need for the quadruple nesting in opening the 4 HDF5 files.
No other change made besides the
with
nesting depth.Reminders
Summary by Sourcery
Enhancements:
save_qgis.py
by using a single context manager for opening four HDF5 files.