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

Problems in parse_string #60

Closed
darkblaze69 opened this Issue Jul 12, 2016 · 6 comments

Comments

Projects
None yet
2 participants
@darkblaze69
Copy link

darkblaze69 commented Jul 12, 2016

Bug 1 :

bitmath.parse_string('7.5KB')
ValueError: The unit KB is not a valid bitmath unit

Bug 2:
Please add a fix to parse strings with only SI units. e.g. 10K, 10M etc.

example:
bitmath.parse_string("4.7MB")
MB(4.7)

bitmath.parse_string("4.7M")
ValueError: The unit M is not a valid bitmath unit

@darkblaze69 darkblaze69 changed the title Parse strings with SI units Problems in parse_string Jul 12, 2016

@tbielawa

This comment has been minimized.

Copy link
Owner

tbielawa commented Jul 13, 2016

Hey @darkblaze69 thanks for your interest.

Bug 1

bitmath.parse_string('7.5KB')
ValueError: The unit KB is not a valid bitmath unit

This is not a bug. KB is not a valid unit in the SI or the NIST system. See the Available Classes and Appendix - On Units sections of the docs for more information on supported units. There is an additional note in the Available Classes section clarifying the kB vs KB situation.

  • Nothing to fix here

Bug 2

Please add a fix to parse strings with only SI units. e.g. 10K, 10M etc.

I'm not so sure about this request. I'm fundamentally torn on the idea. At its heart bitmath is a library intended to finally do units right. The allowed inputs are strict and the output is always accurate and predictable.

Extending parse_string to handle dirty shell units would introduce ambiguity. There is no hard and fast rule that says 10M == 10000000 Bytes or that 10M == 10485760. The unix suffix M has no value without context (B or b).

On the other hand, it might be useful to introduce a new function (or find a way to extend parse_string without breaking current behavior) that is specifically intended for parsing dirty or fuzzy units, like we often get in shell command output. Use of this function would be up to the user, and they'd have to know the inherent risks associated with it. For example, the resulting value may not be accurate. Depending on the shell command or config file value you could encounter a myriad of output formats.

  • 10k (not real)
  • 10K (not real)
  • 10Ki (not real)
  • 10KB (not real)
  • 10kB (real)
  • 10KiB (real)

Maybe I can come up with something that meets your requirements...

  • @darkblaze69 I'll throw together a prototype function with some associated documentation for you to take a look at
@tbielawa

This comment has been minimized.

Copy link
Owner

tbielawa commented Jul 14, 2016

@darkblaze69 I pushed a new branch for testing. It's called shell_unit_parser. The function parse_string_unsafe has unit tests and docs written up. Mind pulling that branch and giving it a few tests?

Docs are viewable here for now, search this file for "parse_string_unsafe": https://raw.githubusercontent.com/tbielawa/bitmath/shell_unit_parser/docsite/source/module.rst

Here's a code block demonstrating the specific examples you listed in your original issue report:

import bitmath
print bitmath.parse_string_unsafe('7.5KB')
7.5 kB
print bitmath.parse_string_unsafe("4.7MB")
4.7 MB
print bitmath.parse_string_unsafe("4.7M")
4.7 MB
@darkblaze69

This comment has been minimized.

Copy link

darkblaze69 commented Jul 16, 2016

@tbielawa looks good to me now, wish this function to be in main package. Thanks

@tbielawa

This comment has been minimized.

Copy link
Owner

tbielawa commented Jul 16, 2016

Ill get to work on that next!

Thanks for testing for me!

You'll get an update in this thread once i submit all the updates

On Jul 16, 2016 12:55 PM, "darkblaze69" notifications@github.com wrote:

@tbielawa https://github.com/tbielawa looks good to me now, wish this
function to be in main package. Thanks


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#60 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AACBYc56E3Un9p4qszXBNNgdQvumTcvxks5qWQ0QgaJpZM4JKkSX
.

@tbielawa

This comment has been minimized.

Copy link
Owner

tbielawa commented Jul 17, 2016

@darkblaze69 New uploads have been pushed to

Docs have been updated quite thoroughly (you're thanked in the news update: http://bitmath.readthedocs.io/en/latest/NEWS.html#bitmath-1-3-1-1 )

Also, I added a toggle-switch so you can force the input to parse as base-2 if you want instead of base-10.

TODO: Create the PPA builds for:

  • Xenial
  • Wily
  • Vivid
  • Precise

TODO: Create the Fedora/EPEL builds for:

  • EPEL 6
  • EPEL 7
  • Fedora 22
  • Fedora 23
  • Fedora 24
@darkblaze69

This comment has been minimized.

Copy link

darkblaze69 commented Jul 17, 2016

great, thanks! killer feature

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