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

ByteSize does not follow the IEEE 1541 standard #592

Open
Alexander882 opened this Issue Oct 30, 2016 · 10 comments

Comments

Projects
None yet
7 participants
@Alexander882

Alexander882 commented Oct 30, 2016

The ByteSize struct uses values such as kilobyte = 1024 byte, whereas the official IEEE 1541 standard dictates that a kilobyte = 1000 bytes, and kibibyte = 1024 bytes, and so on. Wikipedia Link.

Also, the SI unit kilo- is normally shortened with a lowercase k, not a capital K.

I will happily fix these issues myself, but it might be seen as a bit of a breaking change, so I thought I'd hear your opinion first.

@aloisdg

This comment has been minimized.

Show comment
Hide comment
@aloisdg

aloisdg Oct 30, 2016

Collaborator

Hello,

Thank you for pointing this. I think it should be corrected. If it is a breaking change and a good one, we will still add it, just later on.

Collaborator

aloisdg commented Oct 30, 2016

Hello,

Thank you for pointing this. I think it should be corrected. If it is a breaking change and a good one, we will still add it, just later on.

@Alexander882

This comment has been minimized.

Show comment
Hide comment
@Alexander882

Alexander882 Oct 30, 2016

Okay, I'll fix it and send in a pull request with my changes then.

I've also realized a few other things about the ByteSize struct, especially the TryParse method:

  • It does not accept negative values.
  • It throws an exception when passed a null- or empty value. The normal TryParse methods (such as long.TryParse) just return false when passed null etc.

Alexander882 commented Oct 30, 2016

Okay, I'll fix it and send in a pull request with my changes then.

I've also realized a few other things about the ByteSize struct, especially the TryParse method:

  • It does not accept negative values.
  • It throws an exception when passed a null- or empty value. The normal TryParse methods (such as long.TryParse) just return false when passed null etc.
@aloisdg

This comment has been minimized.

Show comment
Hide comment
@aloisdg

aloisdg Oct 30, 2016

Collaborator

Even better. Please dont forget to add unit tests to show all of this.

Collaborator

aloisdg commented Oct 30, 2016

Even better. Please dont forget to add unit tests to show all of this.

@aloisdg aloisdg added the enhancement label Nov 10, 2016

@omar

This comment has been minimized.

Show comment
Hide comment
@omar

omar Nov 25, 2016

Hey guys, I'm the author of ByteSize and peek into this repo every once in a while to see how others use the library and what changes they want.

  • In terms of using IEEE 1541, I agree on that front. When I first created the library I picked what most people are familiar with, even though it's technically wrong. I'm still conflicted about making this change as much as I want. Maybe it would make sense for a v2 release.

  • Negative values: This is an interesting suggestion. I intentionally chose to disallow that behavior thinking it wouldn't be of use. Could you elaborate on a use case for this?

  • TryPrase throwing exceptions: I actually just fixed the TryParse method in v1.2.4.

Thank you for the feedback.

omar commented Nov 25, 2016

Hey guys, I'm the author of ByteSize and peek into this repo every once in a while to see how others use the library and what changes they want.

  • In terms of using IEEE 1541, I agree on that front. When I first created the library I picked what most people are familiar with, even though it's technically wrong. I'm still conflicted about making this change as much as I want. Maybe it would make sense for a v2 release.

  • Negative values: This is an interesting suggestion. I intentionally chose to disallow that behavior thinking it wouldn't be of use. Could you elaborate on a use case for this?

  • TryPrase throwing exceptions: I actually just fixed the TryParse method in v1.2.4.

Thank you for the feedback.

@hangy

This comment has been minimized.

Show comment
Hide comment
@hangy

hangy Nov 28, 2016

Contributor

This is obviously a breaking change, as callers may depend upon the KB values being based on 1024 Bytes instead of 1000 Bytes for whatever calculations. But it could be added as a non-breaking change in a minor release by adding an AppContext feature switch. If the switch is off (default), then 1 KB is 1024 Bytes (current behaviour), and if it's on, then a 1 KB is 1024 Bytes.

Negative values: This is an interesting suggestion. I intentionally chose to disallow that behavior thinking it wouldn't be of use. Could you elaborate on a use case for this?

Negative values could be interesting to display soft disk/mailbox/traffic quota violations.

Contributor

hangy commented Nov 28, 2016

This is obviously a breaking change, as callers may depend upon the KB values being based on 1024 Bytes instead of 1000 Bytes for whatever calculations. But it could be added as a non-breaking change in a minor release by adding an AppContext feature switch. If the switch is off (default), then 1 KB is 1024 Bytes (current behaviour), and if it's on, then a 1 KB is 1024 Bytes.

Negative values: This is an interesting suggestion. I intentionally chose to disallow that behavior thinking it wouldn't be of use. Could you elaborate on a use case for this?

Negative values could be interesting to display soft disk/mailbox/traffic quota violations.

@omar

This comment has been minimized.

Show comment
Hide comment
@omar

omar Dec 1, 2016

@hangy, thanks for the suggestion. I went ahead and did a quick spike of the metric byte, see these lines and the associated test case.

I opted to use a static field on the actual class because it's more self-documenting than AppContext (which requires magic strings). In addition, AppContext is relatively new and is not supported on older versions of .NET.

In terms of negative values and quota violations, generally those are greater than violations. That is:

var quota = ByteSize.FromMegaBytes(20);
var usage = ByteSize.FromMegaBytes(19);
var newFile = ByteSize.FromMegaBytes(2);

if (usage + newFile > quota) {
    throw new InvalidOperationException($"This file would put you above your quota of { quota }");
}

However, I did just realize that you can't subtract two ByteSize objects. I'm not sure why I didn't include that operation so I can do something like this:

throw new InvalidOperationException($"This file would put you above your quota of { quota } by { usage + newFile - quota }");

I'll add that as well as multiplying and dividing. Lastly, ByteSize does store negative bytes, but it doesn't print them out or parse them, but that can be changed.

I'll add those.

omar commented Dec 1, 2016

@hangy, thanks for the suggestion. I went ahead and did a quick spike of the metric byte, see these lines and the associated test case.

I opted to use a static field on the actual class because it's more self-documenting than AppContext (which requires magic strings). In addition, AppContext is relatively new and is not supported on older versions of .NET.

In terms of negative values and quota violations, generally those are greater than violations. That is:

var quota = ByteSize.FromMegaBytes(20);
var usage = ByteSize.FromMegaBytes(19);
var newFile = ByteSize.FromMegaBytes(2);

if (usage + newFile > quota) {
    throw new InvalidOperationException($"This file would put you above your quota of { quota }");
}

However, I did just realize that you can't subtract two ByteSize objects. I'm not sure why I didn't include that operation so I can do something like this:

throw new InvalidOperationException($"This file would put you above your quota of { quota } by { usage + newFile - quota }");

I'll add that as well as multiplying and dividing. Lastly, ByteSize does store negative bytes, but it doesn't print them out or parse them, but that can be changed.

I'll add those.

@onovotny

This comment has been minimized.

Show comment
Hide comment
@onovotny

onovotny Apr 18, 2017

Member

@omar @aloisdg @Alexander882 any chance we can get this nailed down in the next week or so for 2.2? Aiming to get 2.2 out in the beginning of May.

Member

onovotny commented Apr 18, 2017

@omar @aloisdg @Alexander882 any chance we can get this nailed down in the next week or so for 2.2? Aiming to get 2.2 out in the beginning of May.

@mattdwen

This comment has been minimized.

Show comment
Hide comment
@mattdwen

mattdwen Sep 26, 2017

Hi all, any update on this? I'd like to help out with this one if I can as it's something I'd like to use.

mattdwen commented Sep 26, 2017

Hi all, any update on this? I'd like to help out with this one if I can as it's something I'd like to use.

@Alexander882

This comment has been minimized.

Show comment
Hide comment
@Alexander882

Alexander882 Sep 27, 2017

Sorry for being so slow on this one, I've created a pull request with my implementation

Alexander882 commented Sep 27, 2017

Sorry for being so slow on this one, I've created a pull request with my implementation

@MehdiK

This comment has been minimized.

Show comment
Hide comment
@MehdiK

MehdiK Nov 3, 2017

Member

This is a significant breaking change, and like @omar mentioned above, while the current terminology is technically wrong, it is what most people are used to and expect.

Can I suggest that we keep the existing functionality as default behavior, to avoid breaking the behavior, and add a strategy to switch to IEEE standard through config? This is an example of a similar implementation.

Member

MehdiK commented Nov 3, 2017

This is a significant breaking change, and like @omar mentioned above, while the current terminology is technically wrong, it is what most people are used to and expect.

Can I suggest that we keep the existing functionality as default behavior, to avoid breaking the behavior, and add a strategy to switch to IEEE standard through config? This is an example of a similar implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment