internal/units: Create new units package #705
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
rfratto
commented
Apr 29, 2024
rfratto
force-pushed
the
units
branch
2 times, most recently
from
db37b5c to
6285b41
Compare
…ckage This commit creates a replacement for the github.com/alecthomas/units package. A replacement is desirable because the upstream package doesn't work very well with the Alloy syntax: * We use the UnmarshalText implementation of Base2Bytes, which treats metric (MB) and IEC (MiB) suffixes as the same. * Base2Bytes always reports values as IEC suffixes when marshalling them back, which can confuse users into thinking a unit conversion has occurred somewhere. I think it's potentially confusing to users that setting a limit of 4MB actually sets a limit of 4MiB, which is ~4% less than what the user intended. The new implementation supports parsing the same input as the old package, including complex byte sizes such as `4MiB3KiB`, though simplified byte sizes is preferred (`4099KiB`), and the simplified forms are returned when marshaling back into a string.
This switches everything over to the new units package.
mattdurham
reviewed
Apr 29, 2024
mattdurham
reviewed
Apr 29, 2024
|
|
||
| var unitMap = map[string]Bytes{ | ||
| "": Byte, | ||
| "b": Byte, |
There was a problem hiding this comment.
Should this be bit? Which I am not sure how we would even handle.
rfratto
added
the
backport-to-agent:no
|
This PR has not had any activity in the past 30 days, so the |
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.
This commit creates a replacement for the github.com/alecthomas/units package. A replacement is desirable because the upstream package doesn't work very well with the Alloy syntax:
Base2Bytes, which treats metric (MB) and IEC (MiB) suffixes as the same.5 * 1000 * 1000(5MB), it displays as4MiB786KiB832B, which is incredibly hard to read and understand.I think it's confusing to users that setting a limit of
4MBactually sets a limit of4MiB, which is ~4% less than what the user intended.I couldn't find a third party package which supported both IEC and metric while also implementing encoding.TextMarshaler/encoding.TextUnmarshaler, so I wrote a new package.
The new
internal/unitspackage supports both IEC and metric suffixes in the same data type, unmarshaling to the proper type based on what suffix the user provided. Additionally, it willString()to the most appropriate value, a byte count divisible by 1000 shows metric units, otherwise it shows IEC units.The new implementation supports parsing the same input as the old package, including complex byte sizes such as
4MiB3KiB, though simplified byte sizes is preferred (4099KiB), and the simplified forms are returned when marshaling back into a string.