Conversation
| self.vdi = vdi.VDI(f) | ||
| self.vdi = vdi.VDI(fh) | ||
|
|
||
| self._stream = self.vdi.open() |
There was a problem hiding this comment.
As the lifecycle of the stream coincide in vdi in this usecase, and perhaps in most of the usecases, an alternative design would be to let VDI implement stream methods in terms of an internal VDISttream, and provide a factory method to get a fresh VDIStream,
| self.vdi = vdi.VDI(f) | ||
| self.vdi = vdi.VDI(fh) | ||
|
|
||
| self._stream = self.vdi.open() |
There was a problem hiding this comment.
The API suggests a symmetry between vdi.open() and vdi.close() that doesn't actually exist. In reality, the code operates on different levels of abstraction.
The VDI acts as a manager that you shut down using vdi.close() (Level 1).
However, it also acts as a factory to generate results (streams) via vdi.open() (Level 2).
One option would be to rename vdi.open() to vdi.get_stream() or vdi.create_stream().
There was a problem hiding this comment.
Most of these things can be ironed out once we can seriously get into making #100 happen. Currently changes around this are mostly around having the ability to create separate streams from container formats within their implementation. Any changes in dissect.target are currently just bandaid to work with that new API.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1531 +/- ##
==========================================
+ Coverage 80.70% 80.97% +0.26%
==========================================
Files 400 400
Lines 34969 34968 -1
==========================================
+ Hits 28223 28316 +93
+ Misses 6746 6652 -94
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:
|
Merging this PR will not alter performance
Comparing Footnotes
|
Add support for fox-it/dissect.hypervisor#72