Skip to content
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

Convert a Bytes-based object to a human readable string #4

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

marshallmick007
Copy link

Here's an example

b = Humanize::Bytes.new(12345)
k = b.to_human  #returns a Kilo class in this case
k.to_human_s
> "12.06 Kb"

You can also just use the Bytes class to get a humanized string:

b = Humanize::Bytes.new(12345)
b.to_human_s
> "12.06 Kb"

@plribeiro3000
Copy link
Owner

Interesting approach. I would like to merge what you have here but we need to take care of some points before.
The main idea behind the current behaviour is that you need to be explicit about what you have when using Humanize::Bytes api. This gives us something thats tell us whats is happening without reading the implementation code like below:

bytes = Humanize::Byte.new(1024)
bytes.to_p.to_s

This converts the original object to a PetaByte instance then print the new value

With your approach:

bytes = Humanize::Byte.new(1024)
bytes.to_human

We need to pass a value that is big enough to convert to a PetaByte instance to be able to print as PetaByte. It is not as easy to figure out why it got converted to PetaByte in this approach.

Also, if we follow this approach i would like to change it a bit. I would not expect a method called to_human to be converting an object but rather just doing logic needed to print a human readable version of the object. So i prefer to keep using the method to_s as is just to print the object and another method to do the conversion between units. Also to_human_s is not a good name for a method, it is rather confusing.

So what about we get rid of to_human and to_human_s and improve the original to_s method to append the unit as you did on the to_human method?

Also the constants solution is really nice. Maybe we can do that for all objects?
And maybe we can be a little bit more explicit on the constants definitions like:

KILO = 1024**1
MEGA = 1024**2
GIGA = 1024**3
TERA = 1024**4
PETA = 1024**5
EXA = 1024**6
ZETTA = 1024**7
YOTTA = 1024**8

And please don't bump the library version in the same PR as a feature request. Leave it to be done by the maintainer that is going to generate a new release.

Thanks for your interest in making this library better. I hope we can work together to figure the best way to move forward with this!

@marshallmick007
Copy link
Author

Thanks for the feedback!

Sounds like there are really 2 potential use cases to solve for

  1. I have a measurement and I want it printed in a specific measure, for example, I am interesting in measurements in MB only
bytes = Humanize::Byte.new(2000)
bytes.to_m.to_s
=> "0.0019073486328125"

gb = Humanize::Giga.new(3.14)
gb.to_m.to_s
=> "3215.36"
  1. I have an arbitrary set of bytes (or kilobytes, or megabytes, etc) and I want the "nicest" display of those bytes to the user without needing to know what the largest measurement computes to. For the sake of this example, I will use the method name #to_human_s, though I agree it is not the best name in the world.
bytes = Humanize::Byte.new(2000)
bytes.to_human_s
=> "1.95 KB"

bytes = Humanize::Byte.new(123456789)
bytes.to_human_s
=> "117.74 MB"

# This actually doesn't work in the PR
mb = Humanize::Mega.new(3215.36)
mb.to_human_s
=> "3.14 GB"

Since a user creates an object of a specific measure, using #to_s to perform both use cases will not work, since bytes.to_m.to_s would expect the output to be in MB, per item 1, and bytes.to_m.to_s would then not be able to satisfy item 2 if that is an approach you are willing to take. If you wanted to support both cases, we would need to create a new method, perhaps just called #humanize

bytes = Humanize::Byte.new(2000)
bytes.to_m.to_s
=> "0.0019073486328125"
bytes.humanize
=> "1.95 KB"

mb = Humanize::Mega.new(3215.36)
mb.to_s
=> "3215.36"
mb.humanize
=> "3.14 GB"

I will go back to the drawing board on this if you approve. I will submit another PR based on master that constant-izes all of the files

KILO = 1024**1
MEGA = 1024**2
GIGA = 1024**3
TERA = 1024**4
PETA = 1024**5
EXA = 1024**6
ZETTA = 1024**7
YOTTA = 1024**8

@plribeiro3000
Copy link
Owner

I agree with you here. Those are 2 different approaches and rather then changing the current behaviour adding it as a new feature to the library would be awesome.
That means we need a new method but i think we might use the to_s method. What if we create a method that will convert the object to its nicest object for that value?
Then we can chain to_s to print it in a nice and beautiful way.

bytes = Humanize::Byte.new(2000)
bytes.to_m.to_s
=> "0.0019073486328125"
bytes.human.to_s
=> "1.95 KB"

With this approach we will improve the original to_s method to print the unit and have a method to convert the actual object to a value that is more human readable.

WDYT?

@marshallmick007
Copy link
Author

I like it!

Should we close the PR then?

@plribeiro3000
Copy link
Owner

Hmmm. I think you can just push your new changes to The same branch and it
will update automatically with The new version.

Em qua, 5 de out de 2016 12:39, marshallmick007 [email protected]
escreveu:

I like it!

Should we close the PR then?


You are receiving this because you commented.

Reply to this email directly, view it on GitHub
#4 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AApEuDl50WPr3OEHm_RTU6vqNyf8Q-6zks5qw8SbgaJpZM4KM7Sy
.

@marshallmick007
Copy link
Author

Ah, but I was going to go back to your master and ditch my changes and start from scratch so things would be cleaner from your end. Up to you if you want to close this and have me reopen a new PR.

@plribeiro3000
Copy link
Owner

Ah, just get a new branch from mine and do a push force then. =)

Em qua, 5 de out de 2016 12:52, marshallmick007 [email protected]
escreveu:

Ah, but I was going to go back to your master and ditch my changes and
start from scratch so things would be cleaner from your end. Up to you if
you want to close this and have me reopen a new PR.


You are receiving this because you commented.

Reply to this email directly, view it on GitHub
#4 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AApEuFu7KH2J8fATVQEQMUSR8PnXuhQEks5qw8fHgaJpZM4KM7Sy
.

@marshallmick007
Copy link
Author

Would you expect that the to_s method would always output the measurement label like so?

Humanize::Mega.new(10098).human.to_s
=> "9.861328125 GB"
Humanize::Mega.new(10098).to_k.to_s
=> "10340352 KB"
Humanize::Mega.new(10098).to_b.to_s
=> "10588520448 Bytes"
Humanize::Mega.new(10098).to_t.to_s
=> "0.009630203247070312 TB"

One of the reasons I didn't originally re-use the to_s method was to not break pre-existing functionality which would expect to_s to only output a measurement, and not a label. If I implement the above, other applications may end up duplicating labels "10340352 KB KB" since they do not expect to_s to return a label.

The other approach would be to have the human method return a wrapped object and implement to_s on that wrapped object. Perhaps that is what you had meant.

@plribeiro3000
Copy link
Owner

You are right. This would break the current behaviour. What we can do is accept another parameter on to_s that print it in case it is true with default to false.
And in case no value is passed (it means they are using the old way) we can print a deprecation message. This wont break the current behaviour and will give other users time to migrate.
WDYT?

@plribeiro3000
Copy link
Owner

Or Maybe we can create a configuration where they can define if they want it or not.

@marshallmick007
Copy link
Author

marshallmick007 commented Oct 5, 2016

The more I am thinking about this, the more I like the idea of creating a new object whose only responsibility is to find the best object to convert to. This leaves the existing API intact, and adds the new (optional) functionality without polluting the code or base class with unnecessary logic.

class HumanizedMeasurement
   def initialize(measure)
        @measure = humanize(measure)
   end

   def to_s
       @measure.to_s + " " + @measure.label
   end
private
   def humanize(measure)
       # convert measure to the most appropriate object
   end
end

Then in Bytes,

def human
   HumanizedMeasurement.new(self)
end

@plribeiro3000
Copy link
Owner

Dunno. My concern here is that we might be creating something new just for the sake of keeping the current api intact.
Maybe we should just break the current api and bump the MAJOR version. This would allow us to move forward with this change without worrying about compatibility.
Then the logic to find the best human readable unit can be either on the parent object (Bytes) or on a new object.
WDYT?

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