Fix finding cached files for flavor in load() #471
Conversation
Reviewer's Guide by SourceryThe PR fixes a bug in the cache file lookup logic when loading media files with a different format than the original. The implementation introduces a new helper function Class diagram for updated cache file lookup logicclassDiagram
class Load {
+_missing_files(files, files_type, db_root, flavor, verbose)
}
class Load {
+is_cached(file) bool
}
Load --|> is_cached : uses
note for is_cached "Helper function to check if a file is cached, considering format conversion for media files."
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
Review
The changes are quite localized: the private function missing_files is exclusively used by another private function _load_files.
Therefore, for reviewing it makes sense to focus on the tests.
As the method is directly callable it also can be tested in isolation.
The only concern one might raise is that
audb.core.load._missing_files is only called with one parameter combination.
I am assuming that this is ok and will approve the changes.
|
The other parameter combinations are tested implicitly by other functions. Usually, we do not write extra tests for private functions, but the exceptions are if there concrete bugs that need to be fixed as it is the case here. Hence, I added an explicite test for |
Closes #324
Ensures that media files are found in cache when using
audb.load(..., format=format)andformatis different from the original format of the media files. This is achieved by replacing the file extension of the original file (original file names are given by dependency table) by the givenformatwhen checking if the file exists in the cache. The pull request also adds a test for the expected behavior, which is failing for the currentmainbranch./cc @ureichel