Added :prefix => :iec option #7835

Closed
wants to merge 7 commits into
from

Projects

None yet

9 participants

@jarl-dk
jarl-dk commented Oct 3, 2012

This will use base 1024 and the notation (KiB, MiB, GiB, etc.)

http://en.wikipedia.org/wiki/Binary_prefix

Also corrected the SI prefix for thousands, SI states lowercase "k" not capital letter "K"

@jarl-dk jarl-dk Added :prefix => :iec option
This will use base 1024 and the notation (KiB, MiB, GiB, etc.)

    http://en.wikipedia.org/wiki/Binary_prefix

Also corrected the SI prefix for thousands, SI states small-letter "k"
not kapital letter "K"
1094acd
@jarl-dk
jarl-dk commented Oct 3, 2012

supersedes #7829

@jarl-dk
jarl-dk commented Oct 3, 2012

I would be really happy if someone could implement something like a default prefix as proposed in bottom of #7829 (comment), I just don't know where that kind of thing should be put.

@yfeldblum

The options key should be renamed from :prefix to :units.

The reason is that what this commit actually talks about is a choice among multiple units of measurement or systems of measurement; and this commit is not really talking about in which sequence are the number and the unit of measure printed. In English, the units of measure usually comes as a :suffix, so the word :prefix is odd anyway.

@jarl-dk
jarl-dk commented Oct 3, 2012

If the option key is renamed backward compatibility is broken (for all those rails apps that are already using :prefix => :si option), but apart from that I don't have any opinion about it. Maybe a different pull request should be created for that change.

@rafaelfranca rafaelfranca commented on an outdated diff Oct 3, 2012
activesupport/lib/active_support/number_helper.rb
@@ -1,3 +1,4 @@
+# -*- coding: utf-8 -*-
@rafaelfranca
rafaelfranca Oct 3, 2012 Ruby on Rails member

Why you added this?

@jarl-dk
jarl-dk commented Oct 3, 2012

@rafaelfranca : It was added by my editor (I overlooked), sorry.

@jarl-dk
jarl-dk commented Oct 3, 2012

Thanks, @rafaelfranca, I'll take a look at it later (bed time in Denmark)

@yfeldblum

@jarl-dk Thanks for the clarification about :prefix.

As another commit in this pull-request, since you're already working on this code, perhaps you can add a make the real option in rails-4 be :units but also make :prefix be a synonym of :units for backward compatibility?

Also I would go back to using :binary instead of :customary since "binary prefix" is actually the name of the customary units of measurement.

(In this case the word "prefix" does not describe the units with relation to the number, but only describes the order-of-magnitude part of the units (e.g., mega) with relation to the dimension (e.g., bytes).)

@rafaelfranca rafaelfranca commented on an outdated diff Oct 3, 2012
actionpack/test/template/number_helper_test.rb
assert_equal '1.23 MB', number_to_human_size(1234567, :prefix => :si)
assert_equal '1.23 GB', number_to_human_size(1234567890, :prefix => :si)
assert_equal '1.23 TB', number_to_human_size(1234567890123, :prefix => :si)
end
+ def test_number_to_human_size_with_iec_prefix
+ assert_equal '3 Bytes', number_to_human_size(3.14159265, :prefix => :iec)
+ assert_equal '123 Bytes', number_to_human_size(123.0, :prefix => :iec)
+ assert_equal '123 Bytes', number_to_human_size(123, :prefix => :iec)
+ assert_equal '1.21 KiB', number_to_human_size(1234, :prefix => :iec)
@rafaelfranca
rafaelfranca Oct 3, 2012 Ruby on Rails member

Fix the indentation

@rafaelfranca rafaelfranca commented on an outdated diff Oct 3, 2012
activesupport/test/number_helper_test.rb
assert_equal '1.23 MB', number_helper.number_to_human_size(1234567, :prefix => :si)
assert_equal '1.23 GB', number_helper.number_to_human_size(1234567890, :prefix => :si)
assert_equal '1.23 TB', number_helper.number_to_human_size(1234567890123, :prefix => :si)
end
end
+ def test_number_to_human_size_with_iec_prefix
+ [@instance_with_helpers, TestClassWithClassNumberHelpers, ActiveSupport::NumberHelper].each do |number_helper|
+ assert_equal '3 Bytes', number_helper.number_to_human_size(3.14159265, :prefix => :iec)
+ assert_equal '123 Bytes', number_helper.number_to_human_size(123.0, :prefix => :iec)
+ assert_equal '123 Bytes', number_helper.number_to_human_size(123, :prefix => :iec)
+ assert_equal '1.21 KiB', number_helper.number_to_human_size(1234, :prefix => :iec)
@rafaelfranca
rafaelfranca Oct 3, 2012 Ruby on Rails member

Fix the indentation

@jarl-dk
jarl-dk commented Oct 3, 2012

@yfeldblum : as the article describes (in the very first paragraph) binary is the name for any prefix/notation using 1024 as exponent base, so IEC is also a binary prefix/notation. So binary is too generic to use in this case as it covers two out of three cases, that is why I choose customary, since both JEDEC mentions the use of KB in their publication, and the whole wiki article uses the word "customary" all places where it is necessary to differentiate between IEC and "customary" prefix/notation (when speaking of binary prefix).

@yfeldblum

@jarl-dk That's a good point. You've proved me wrong and now I agree with your choice of :customary. Thank you!

@jarl-dk
jarl-dk commented Oct 3, 2012

@yfeldblum : Anyway, few will ever use :customary, because it will be default. Only in case where they specify something else (say :iec) as default on something like ActiveSupport::default_size_prefix = :iec (code yet to come) and then on individual calls needs the default (customary) notation, then they will need to specify :prefix => :customary in options (anyway specifying something wrong e.g. a typo will result in a helpful exception)

@yfeldblum

@jarl-dk Yep, I'm with you on that.

@jarl-dk jarl-dk Added ActiveSupport.default_size_prefix to be used when no :prexfix
option is specified. Default value is :customary

Added :prefix => :iso to be equivalent to :prefix => :iec, made :iso
the canonical name for the ISO/IEC prefix notation in test methods
names since ISO is a wider standards organisation.
6bc5d19
@jarl-dk
jarl-dk commented Oct 4, 2012

The reason I chose default_size_prefix is that
the method is named number_to_human_size, so I guess "size" is the term used for this digital quantity, and 'prefix' because that is the name of the option.

@jarl-dk
jarl-dk commented Oct 4, 2012

Regardnig :prefix => versus :units => (the name of the option): I would actually argue that it should stay as :prefix because the unit is in all three cases bytes. The difference is actually the prefix system, whether it is a binary customary prefix system or binary ISO prefix system, or whether it is SI prefix system. So I hope it will stay as :prefix. Anyway it should stay that way right now due to API stabilisation.

@jarl-dk
jarl-dk commented Oct 10, 2012

Ping... Any feedback on the latest commit...

@jarl-dk
jarl-dk commented Oct 18, 2012

So is someone willing to merge this?

@calmyournerves

Would like to see this merged!

@rafaelfranca
Member

We are discussing this. I'll try to get a answer this week

@jeremy
Member
jeremy commented Oct 20, 2012

@jarl-dk The :customary name makes sense in a wikipedia article but doesn't make sense to users. That's something you'd have to google to understand. Same with ActiveSupport.default_size_prefix -- seeing that in isolation, I'd have no idea what it means or what it applies to.

Sorry this review is so rocky. I think your expansion of units/prefixes supported has exposed our ambivalence toward adding additional complexity to this helper method. It's way overloaded with responsibility, doing too much, and even exposes a toplevel ActiveSupport method to configure it.

That points toward either extracting the helper method entirely (and reverting :si prefix support) to a separate library, or coming up with a clearer design for dealing with bytes. For example, a Bytes class that understands units and formatting, much like how we added Duration to deal with times.

Then we'd have Bytes#to_s(units = Bytes.default_units) that you could override like 1000.bytes.to_s(:si).

@jarl-dk
jarl-dk commented Oct 21, 2012

@jeremy : Thanks for letting me know...
Regarding :customary I think googling it is mostly necessary for users that do not speak english natively. Assuming that this is natural association for you, I think :customary makes sense enough. But the issues is not important to me...

Regarding the ActiveSupport.default_size_prefix: I dislike this name too... As I explained in #7835 (comment) The name was derived from the helper method name (number_to_human_size) , which I also doesn't feel is a very good description of the functionality/responsibility, but I was focusing at one thing at a time.

Regarding the design/complexity: Maybe a Red-Green-Refactor approach should be taken: This PR contains the Red step (introducing tests) and the Green step (introducing the implementaiton), but not the Refactor step. Refactoring the implementation could be done in other Pull Requests, or just straight commits (for those who have commit privileges). Then it would also be clear which functionality (API) is changed seperately from which functionality (API) is extended (which is done in this PR).

BTW: Where is this discussion taking place: some mailing-list or IRC?

Anyway I will probably be happy with whatever you come up with. It just wasn't visible to me that the PR was under discussion.

@frodsan frodsan and 1 other commented on an outdated diff Oct 26, 2012
actionpack/test/template/number_helper_test.rb
@@ -179,13 +179,24 @@ def test_number_to_human_size_with_si_prefix
assert_equal '3 Bytes', number_to_human_size(3.14159265, :prefix => :si)
assert_equal '123 Bytes', number_to_human_size(123.0, :prefix => :si)
assert_equal '123 Bytes', number_to_human_size(123, :prefix => :si)
- assert_equal '1.23 KB', number_to_human_size(1234, :prefix => :si)
- assert_equal '12.3 KB', number_to_human_size(12345, :prefix => :si)
+ assert_equal '1.23 kB', number_to_human_size(1234, :prefix => :si)
@frodsan
frodsan Oct 26, 2012

Please, use 1.9 hash syntax, thanks :)

@vinnydiehl
vinnydiehl Oct 26, 2012

Agreed on 1.9 hash syntax.

@jarl-dk
jarl-dk commented Oct 26, 2012

I don't mind changing it to 1.9 hash syntax. But I don't quite understand... All the rest of the file is 1.8 syntax, should I change the whole file or just the places that I edited.

@frodsan
frodsan commented Oct 26, 2012

Just on the places you edit or add new code. We don't have an agressive plan to change all the code yet. Thanks.

@steveklabnik
Member

@jarl-dk the problem is that if we mass change the syntax everywhere, we lose the historical information about when things changed. A massive diff that may make things pretty but obscures history, and causes everyone else's open changes to need a rebase. So we just use the new syntax with new code, and when touching old code.

@jarl-dk
jarl-dk commented Oct 27, 2012

@steveklabnik : Thanks for enlightening me. Makes perfect sense... However in such case people will not only need to rebase (or merge), they will experience merge conflicts in the process. So for sure not a good approach.

@jarl-dk
jarl-dk commented Dec 20, 2012

So. Does anyone have the courage to merge this PR? please...

@rafaelfranca
Member

I really think this should not be merged. First, I'd remove the :si prefix. Later, I'd extract this from Rails and put in a gem.

I think this is time to decide what we will do with this pull request.

@jeremy @dhh I know you guys have strong opinion about this one, how should we proceed?

@steveklabnik
Member

I'd 👍 that notion, due to the ambivalence of almost everyone involved.

@dhh
Member
dhh commented Dec 20, 2012

+1 on killing the :si option and kicking all this config off to a gem. Let Rails just keep the human names and a gem can worry about being :neckbeard: compatible.

@dhh dhh closed this Dec 20, 2012
@rafaelfranca
Member

@jarl-dk thank you very much for your awesome work but we think this should not be part of the framework.

@jarl-dk
jarl-dk commented Dec 21, 2012

@rafaelfranca, @steveklabnik: Thanks for elaborating your opinions about this issue...

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