Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1589 +/- ##
==========================================
+ Coverage 81.02% 81.09% +0.06%
==========================================
Files 400 401 +1
Lines 34965 34988 +23
==========================================
+ Hits 28332 28372 +40
+ Misses 6633 6616 -17
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| # We should only have one candidate at this point | ||
| if len(candidates) > 1: |
There was a problem hiding this comment.
This is the only change that does not make sense to me right now. Why are we not iterating candidates and loading the first "valid" one anymore?
There was a problem hiding this comment.
It's been a while since I looked at VBK and I don't have access to any data anymore, so I might just be forgetting something.
There was a problem hiding this comment.
This current setup doesn't really allow it because we can only return a single path, and I can't really think of a nicer way to allow that way, except for changing prepare to be an iterator instead of simply returning a path. I can maybe imagine a few more usecases where that could be useful, so maybe it's worth changing to that. Perhaps the reviewer can weigh in too.
But if I understand the logic in the VBK loader correctly, the candidates list should contain paths only if the globs for either vmx or vmcx succeeded, and otherwise the multiloader path. I don't expect there to be a valid way to create a VBK that contains both a vmx and a vmcx file.
There was a problem hiding this comment.
Allright, makes sense to me.
|
Other than that, it looks good to me. Seems like this would make sure you never have to look for a new loader within a loader anymore. |
Consider this a proposal/experiment for a middleware loader, which should facilitate making loaders like #1456 and #1214 easier. I've adjusted the VBK loader as test (which also immediately serves as a unit test).
@qmadev I remember you looked a bit at VBK too, besides the middleware loader itself, does the VBK loader change make sense to you?