Adding TransferSpeedColumn configuration to display bits/bytes + binary/decimal prefixes

[tpill90]

Context

Currently the TransferSpeedColumn is displaying the transfer speed using the binary/IEC definition of a kilobyte/megabyte/gigabyte (1024^x)

Typically, network transfer speeds are specified using the decimal definition (1000^x), for example Ethernet (10/100/1000BASE-T), or internet connection speeds (100mbit = 100,000,000,000)

https://en.wikipedia.org/wiki/Binary_prefix#Data_transmission_and_clock_rates

Describe the solution you'd like

I would like to propose adding the ability to configure which binary prefix (1000^x or 1024^x) is used for display, as well as configuring to display either bits/s or bytes/s. This would allow developers to be able to tweak formatting to better match their use case.

The default display format would be to continue to use the current format, 1024^x , as to ensure that there are no unexpected changes to the existing behavior when updating to a newer version of the package.

Using a data transfer rate of 1,000,000 bytes, the expected output for each configurablee option would be as follows :

Binary Prefix	Bits/Bytes	Expected Output
Binary	bytes	.95 MiB/s
Decimal	bytes	1 MB/s
Binary	bits	7.63 Mibit/s
Decimal	bits	8 Mbit/s

Testing

These changes have been in use for about a month now in my projects steam-lancache-prefill and battlenet-lancache-prefill, and have been able to be tested and proven to be working, as well as being able to smooth over any unexpected edge cases.

Example output of the change :

Related commit, where changes started to be used

---

Please upvote 👍 this pull request if you are interested in it.

[CLAassistant]

All committers have signed the CLA.

[tpill90]

@patriksvensson @phil-scott-78 Could I kindly ask for your thoughts and feedback on these changes whenever you have some time to take a look? I'd be more than happy to discuss the changes in further details, and I would be glad to know if there is anything I can do to help get these changes included. Thank you!

[tpill90]

@microsoft-github-policy-service agree

[FrankRay78]

Hey @tpill90, looks like an interesting, well drafted PR - are you still around to help me get this reviewed/merged? (assuming I might need your help, although perhaps we get lucky with a straight merge...). I assume you'd like this for https://github.com/tpill90/steam-lancache-prefill ? (which looks awesome btw)?

Update
An initial cursory glance seems to reveal Live\Progress\Columns\TransferSpeedColumn missing test coverage (in main branch, not your PR). You've done a great job of updating DownloadColumnTests, and it would seem reasonable to take this opportunity to retrofit some initial tests for TransferSpeedColumn as well.

[FrankRay78]

I like this PR @tpill90 and have started reviewing it. Solid code.

[FrankRay78]

Initial review feedback and suggested changes here: https://github.com/tpill90/spectre.console/pull/1

[FrankRay78]

Brain dump of my review comments below.

I used to dread this kind of thing when contributing @tpill90, but the review is a sign that I feel your contribution is worth spending time on, and getting it over the line.

I'm not 'the boss' and you don't need to silently accept everything I say in this, rather, we are peers, your coding skills are probably superior to mine, and we hash out the details until we are both happy. Truly excellent library code is what we want. Feel free to disagree with anything I say, but provide an explanation when you do, so I can consider the merits.

Your fork/branch is really old, I'd suggest you rebase (as I can't actually compile it - I've been working off a recent main fork with your commits cherry-picked).

Noted your comment about testing TransferSpeedColumn - I'll need to give it some thought.

[FrankRay78]

There is no rush/pressure @tpill90 with this. Just to say I will be happy to progress the PR as and when you are ready. It won't be one that languishes. In the meantime, be well 👏.

[tpill90]

Hey Frank, I apologize for not getting a chance to look at it yet. I'm going to try to get to it today. Thanks for your help on this

[FrankRay78]

I'll look at your comments/changes soon @tpill90 👏

[FrankRay78]

FYI. The build is broken on your branch, following the rename from DisplayBits to ShowBits, see:

[tpill90]

I pushed up another commit which should take care of the latest comments you've left on the review @FrankRay78 .

A follow up question, what documentation should I be updating with this PR? I'll gladly update it, I just was waiting for things to get closer to being merged before going through all of it 😄

[FrankRay78]

I've been thinking about documentation. Strictly speaking, the xml comments on things like the ShowBits property will end up in the auto generated API reference, however I notice the Progress page code example doesn't include the Download progress column, see: https://spectreconsole.net/live/progress

I think it might be enough to just add that into the example, so it becomes obvious to casual readers that such a thing exists.

[FrankRay78]

Frigging awesome coding @tpill90, I think we are good with the PR review.

I'm just looking at the documentation and playing around with some exploratory use of the Download/TransferSpeed columns to get a better idea of what we might want to explain to users.

Feel free to do the same, and/or I'll write my suggestion once done. We can always discuss if we have differences of opinion.

[FrankRay78]

FYI. I've suggested a couple of final changes, here to consider, prior to merging.