-
Notifications
You must be signed in to change notification settings - Fork 1
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
Improve code readability of audb.publish() #493
Conversation
Reviewer's Guide by SourceryThis pull request improves the code readability of the Sequence diagram for media file handling in _put_media()sequenceDiagram
participant C as Client
participant PM as _put_media()
participant BE as Backend Interface
participant FS as File System
C->>PM: Upload media archives
activate PM
Note over PM: Map archives to files
loop For each archive
alt Previous version exists & missing files
PM->>BE: get_archive()
BE-->>PM: archive content
PM->>FS: Copy missing files
end
PM->>BE: put_archive()
BE-->>PM: confirmation
PM->>PM: Update dependency versions
end
PM-->>C: Complete
deactivate PM
Sequence diagram for media file processing in _find_media()sequenceDiagram
participant C as Client
participant FM as _find_media()
participant FS as File System
participant DT as Dependencies Table
C->>FM: Process media files
activate FM
FM->>DT: Remove old dependencies
loop For each media file
alt New media file
FM->>FS: Calculate checksum
FM->>DT: Add new entry
else Existing media file
FM->>FS: Calculate checksum
alt Checksum changed
FM->>DT: Update entry
end
end
end
FM-->>C: Return updated archives
deactivate FM
Class diagram showing the refactored structureclassDiagram
class _find_media {
+process_new_media(file: str)
+process_existing_media(file: str)
+job(file: str)
}
class _put_media {
+upload_archive(archive: str)
}
class Dependencies {
+_drop(files)
+_update_media()
+_add_media()
+_update_media_version()
}
_find_media ..> Dependencies
_put_media ..> Dependencies
note for _find_media "Improved with better structure and documentation"
note for _put_media "Enhanced error handling and clearer organization"
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
@sourcery-ai review |
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 @hagenw - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 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.
Hey @hagenw - 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.
Remaining discussions have been resolved.
The comments facilitate developing an understanding of the publish mechanism.
I therefore think that it is justified to give approval.
Closes #53
Improves the code structure for the private functions
_find_media()
and_put_media()
insideaudb.publish()
.It also adds docstrings and further comments to the private methods, to make it easier to understand them.
In addition, it adds a custom
RuntimeError
when failing to download missing media files from a previous version in_put_media()
. As this case will happen extremely rarely in reality and it would be very complicated to create a test for it, I did not added a new test for it.Summary by Sourcery
Improve the structure and documentation of the
audb.publish()
method, specifically the_find_media()
and_put_media()
private functions. Add aRuntimeError
for a rare edge case in_put_media()
when downloading missing media files.Enhancements: