Skip to content

Bump bytesize from major version 1 to 2 #33

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 13, 2025

Conversation

EliahKagan
Copy link
Member

@EliahKagan EliahKagan commented Apr 13, 2025

This upgrades the bytesize dependency in Cargo.toml from version 1.0.1 (which was usually selecting 1.3.3) to 2.0.1 (which is the latest version).

There were some nontrivial API changes from major version 1 to 2. Accordingly, this adapts Bytes::format_bytes() to the API change.

The old bytesize::to_string() function was confusing in its second parameter si_prefix: bool, which is why it was removed. But it looks like a value of false, as we were passing, actually caused decimal SI units (rather than binary IEC units) to be used. It's not obvious which units prodash intended to use bytesize to convert to and display in. But this seems to be a minimal change to adapt to the new major version.

In spite of the name si_prefix for the old parameter, experiments show that setting it to true caused values to be converted to and presented in binary IEC units.

Although this attempts not to change which actual units are used, it does produce observable differences for some of them, in how they are presented. In particular, a decimal SI kilobyte is least ambiguously abbreviated "kB" (because "k" is an SI unit symbol prefix for "kilo" in its meaning of 1000), but it was previously written as "KB". It is now expected to be writen as "kB". Tests catch this distinction, and are updated here accordingly to assert that the generally preferable "kB" symbol for decimal SI kilobyte is used.

See also: GitoxideLabs/gitoxide#1947 (comment)


A few ways this might not be ready:

  • As noted, I'm not sure what the actual intended behavior is here in terms of whether the units should be the decimal SI units or binary IEC units. My preference for units that are based in counts of bytes is, in most applications, to use binary IEC units--but that does not mean it's what people expect here, and it's a bigger change from what was done before so I did not change to that here, instead using the decimal SI units.
  • There's a behavioral change here, which required a change to the tests--writing "kB" instead of "KB"--so it seems like maybe this should be a conventional commit, but I haven't made it one, not being sure what is expected here.
  • It might be good to try this out in a real-world application, such as by building gitoxide against prodash with these changes and observing what gix clone on a medium-to-large repository looks like.

This upgrades the `bytesize` dependency in `Cargo.toml` from
version 1.0.1 (which was usually selecting 1.3.3) to 2.0.1 (which
is the latest version).

There were some nontrivial API changes from major version 1 to 2.
Accordingly, this adapts `Bytes::format_bytes()` to the API change.

The old `bytesize::to_string()` function was confusing in its
second parameter `si_prefix: bool`, which is why it was removed.
But it looks like a value of `false`, as we were passing, actually
caused decimal SI units (rather than binary IEC units) to be used.
It's not obvious which units `prodash` intended to use `bytesize`
to convert to and display in. But this seems to be a minimal change
to adapt to the new major version.

In spite of the name `si_prefix` for the old parameter, experiments
show that setting it to `true` caused values to be converted to and
presented in binary IEC units.

Although this attempts not to change which actual units are used,
it does produce observable differences for some of them, in how
they are presented. In particular, a decimal SI kilobyte is least
ambiguously abbreviated "kB" (because "k" is an SI unit symbol
prefix for "kilo" in its meaning of 1000), but it was previously
written as "KB". It is now expected to be writen as "kB". Tests
catch this distinction, and are updated here accordingly to assert
that the generally preferable "kB" symbol for decimal SI kilobyte
is used.

See also:
GitoxideLabs/gitoxide#1947 (comment)
Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot, much appreciated.

I don't think I ever much cared about subtleties in how byte-counts are presented, so let's go with this and remove bytesize from the dependency tree of gitoxide.

@Byron Byron merged commit 4ff81bf into GitoxideLabs:main Apr 13, 2025
2 checks passed
@EliahKagan
Copy link
Member Author

I don't think I ever much cared about subtleties in how byte-counts are presented, so let's go with this and remove bytesize from the dependency tree of gitoxide.

Can you elaborate? Since all versions of prodash depend on some version of bytesize, and gitoxide uses prodash, presumably bytesize would have to remain somewhere in the dependency tree of gitoxide.

It occurs to me that you may mean that gitoxide should only ever depend on bytesize through prodash. But I don't think prodash exposes all of it, does it? In any case, gitoxide-core uses bytesize directly in at least one place: to produce statistics for gix free pack verify. Since this isn't related to the functionality of prodash, it seems to me that it remains defensible to have that separate dependency.

Alternatively, do you mean simply that we should remove the old, major-version-1 of bytesize from the dependency tree of gitoxide? That should certainly be feasible, I think.

@EliahKagan EliahKagan deleted the bump-bytesize branch April 13, 2025 09:34
@Byron
Copy link
Member

Byron commented Apr 13, 2025

Sorry, I meant bytesize@1 was supposed to be removed from the dependency tree - right now prodash is the reason both versions are pulled in.

@EliahKagan
Copy link
Member Author

No problem, and thanks for clarifying!

@EliahKagan
Copy link
Member Author

I'll open a PR in gitoxide to update prodash, or cause Dependabot to open one (the cadence is monthly, but it can be manually triggered at any time).

@EliahKagan
Copy link
Member Author

I've upgraded prodash in GitoxideLabs/gitoxide#1953.

EliahKagan added a commit to EliahKagan/gitoxide that referenced this pull request Apr 14, 2025
`gix-features` republishes `bytesize`, which has been bumped from
major version 1 to major version 2. Because the interface of
`bytesize` is effectively part of that of `gix-features` due to
the explicitly republication of the `bytesize` module in full with
no caveats, this is effectively a breaking change in `gix-features`
as well, though many callers may not be substantially affected.

Major changes that may affect some callers that use `bytesize`
through `gix-features` include the removal of the top-level
`bytesize::to_string()` function, the change in default behavior
from displaying decimal SI units to displaying binary IEC units
(though all or most gitoxide-related creates, in adapting to the
changes, have thus far opted to continue using decimal SI units),
and the small but UI-facing change that decimal SI kilobytes
(units of 1000 bytes) are given with the symbol "kB" rather than
the more ambiguous (and arguably less accurate) symbol "KB".

In addition to republishing `bytesize`, `gix-features` also
republishes `prodash`. Futhermore, some uses of `bytesize` are
transitively through `prodash`, which itself has recently received
an update to use `bytesize` major version 2. (Since `prodash` does
not republish `bytesize`, that is not considered to be a breaking
change in `prodash` itself.) To get the benefits of the newer
version of `bytesize` while avoiding new inconsistencies, and also
to avoid depending on multiple versions of `bytesize`, the
`prodash` dependency version has also been upgraded.

For more information, see:

- GitoxideLabs#1952
- https://github.com/bytesize-rs/bytesize/releases/tag/bytesize-v2.0.0
- GitoxideLabs#1949
- GitoxideLabs/prodash#33
- GitoxideLabs#1953
EliahKagan added a commit to EliahKagan/gitoxide that referenced this pull request Apr 14, 2025
`gix-features` republishes `bytesize`, which has been bumped from
major version 1 to major version 2. Because the interface of
`bytesize` is effectively part of that of `gix-features` due to
the explicitly republication of the `bytesize` module in full with
no caveats, this is effectively a breaking change in `gix-features`
as well, though many callers may not be substantially affected.

Major changes that may affect some callers that use `bytesize`
through `gix-features` include the removal of the top-level
`bytesize::to_string()` function, the change in default behavior
from displaying decimal SI units to displaying binary IEC units
(though all or most gitoxide-related creates, in adapting to the
changes, have thus far opted to continue using decimal SI units),
and the small but UI-facing change that decimal SI kilobytes
(units of 1000 bytes) are given with the symbol "kB" rather than
the more ambiguous (and arguably less accurate) symbol "KB".

In addition to republishing `bytesize`, `gix-features` also
republishes `prodash`. Futhermore, some uses of `bytesize` are
transitively through `prodash`, which itself has recently received
an update to use `bytesize` major version 2. (Since `prodash` does
not republish `bytesize`, that is not considered to be a breaking
change in `prodash` itself.) To get the benefits of the newer
version of `bytesize` while avoiding new inconsistencies, and also
to avoid depending on multiple versions of `bytesize`, the
`prodash` dependency version has also been upgraded.

For more information, see:

- GitoxideLabs#1952
- https://github.com/bytesize-rs/bytesize/releases/tag/bytesize-v2.0.0
- GitoxideLabs#1949
- GitoxideLabs/prodash#33
- GitoxideLabs#1953
EliahKagan added a commit to EliahKagan/gitoxide that referenced this pull request Apr 15, 2025
`gix-features` republishes `bytesize`, which has been bumped from
major version 1 to major version 2. Because the interface of
`bytesize` is effectively part of that of `gix-features` due to
the explicit republication of the `bytesize` module in full (with
no documented extra limitations related to interface stability),
this is effectively a breaking change in `gix-features` as well,
though many callers may not be substantially affected.

Major changes that may affect some callers that use `bytesize`
through `gix-features` include the removal of the top-level
`bytesize::to_string()` function, the change in default behavior
from displaying decimal SI units to displaying binary IEC units
(though all or most gitoxide-related creates, in adapting to the
changes, have thus far opted to continue using decimal SI units),
and the small but UI-facing change that decimal SI kilobytes
(units of 1000 bytes) are given with the symbol "kB" rather than
the more ambiguous (and arguably less accurate) symbol "KB".

In addition to republishing `bytesize`, `gix-features` also
republishes `prodash`. Futhermore, some uses of `bytesize` are
transitively through `prodash`, which itself has recently received
an update to use `bytesize` major version 2. (Since `prodash` does
not republish `bytesize`, that is not considered to be a breaking
change in `prodash` itself.) To get the benefits of the newer
version of `bytesize` while avoiding new inconsistencies, and also
to avoid depending on multiple versions of `bytesize`, the
`prodash` dependency version has also been upgraded.

For more information, see:

- GitoxideLabs#1952
- https://github.com/bytesize-rs/bytesize/releases/tag/bytesize-v2.0.0
- GitoxideLabs#1949
- GitoxideLabs/prodash#33
- GitoxideLabs#1953
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants