Improve convert interface #47
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Creating a converter and calling converter.Convert() feels too
complicated for no reason. We need to keep state during the conversion,
but the user does not need to care about the internals.
Since we already made incompatible changes and clients like lima need to
change, this is an opportunity to get the interface right so future
changes can be backward compatible.
Changes:
- `Convert()` is now a function accepting `Options`, saving one step for
the caller.
- The progress argument is now a `Progress` option, so users do not need
to pass nil to disable progress.
- The size argument was removed since we can use the size of the image.
If we want to support a byte range we can add start and length
options without changing the function signature.
- `Converter` renamed to `conversion`, created inside `Convert()`
- Since we create a new `conversion` for each call, we don't need
`reset()`
- Adding future options will not change the function signature
Example usage using defaults:
convert.Convert(target, img, convert.Options{})
Example usage with progress bar:
convert.Convert(target, img, convert.Options{Progress: bar})
Signed-off-by: Nir Soffer <[email protected]>
nirs
added a commit
to nirs/lima
that referenced
this pull request
Nov 21, 2024
For testing: lima-vm/go-qcow2reader#47 Signed-off-by: Nir Soffer <[email protected]>
nirs
added a commit
to nirs/lima
that referenced
this pull request
Nov 21, 2024
We cannot track conversion progress using the image, since convert reads only the allocated extents of the image. We need to pass the progress bar using convert.Options. pb.ProgressBar need to be adapted to convert.Updater interface, so progressbar returns now our own type. This can be used later for other improvement like hiding the progress bar. Testing lima-vm/go-qcow2reader#47 Signed-off-by: Nir Soffer <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Creating a converter and calling converter.Convert() feels too complicated for no reason. We need to keep state during the conversion, but the user does not need to care about the internals.
Since we already made incompatible changes and clients like lima need to change, this is an opportunity to get the interface right so future changes can be backward compatible.
Changes:
Convert()is now a function acceptingOptions, saving one step for the caller.Progressoption, so users do not need to pass nil to disable progress.Converterrenamed toconversion, created insideConvert()conversionfor each call, we don't needreset()Example usage using defaults:
Example usage with progress bar:
Lima pr: lima-vm/lima#2933