Repository: PyFilesystem/pyfilesystem2
Pull request #85 (2017-10-04 12:13:31)

Fix units in `fs.filesize`
According to [Wikipedia](https://en.wikipedia.org/wiki/Byte), the binary filesize units should be `KiB`, `MiB`, ..., and the decimal units should be `kB`, `MB`, ...

Previously, `traditional` would use the SI prefixes, and `decimal` would display bits instead of bytes.

Also, I'm not sure about this, but once again functions should probably be raising `TypeError` when given an argument that cannot be coerced to a numerical value.

coveralls: 
[![Coverage Status](https://coveralls.io/builds/13563357/badge)](https://coveralls.io/builds/13563357)

Coverage remained the same at 99.949% when pulling **f02c4473491736d508b352b0ea36733dd32a61d3 on althonos:patch-1** into **7a274da5963f5dd77e9a953de7d0415d1df9f75e on PyFilesystem:master**.

willmcgugan: I was using https://en.wikipedia.org/wiki/File_size as a guide, which appears to contradict with that.

It's all very confusing, tbh.

Think you're right re ValueError -> TypeError.
althonos: I tried to read https://en.wikipedia.org/wiki/Binary_prefix and it did not make more sense:

If you check the [IEC](https://en.wikipedia.org/wiki/ISO/IEC_80000#Information_science_and_technology), it enforces the use of `Ki`, `Mi`, etc. for binary sizes, but the JDEC goes the other way around, that's why nobody is trying to make things clearer.

But it seems like `Kibi` means `Kilobinary`, so I guess it should be used with binary.


willmcgugan: The original implementation matches this [django filter](https://docs.djangoproject.com/en/1.11/ref/templates/builtins/#filesizeformat).

I'm thinking that perhaps filesize.py doesn't belong here. I use it in `moya serve` when rendering directory listings, but it isn't referenced anywhere in `fs`.

Are you using it anywhere? Tempted to just remove if from the lib to sidestep this thorny issue.
althonos: I wouldn't remove it, since this is the kind of code you keep on rewriting and rewriting again everytime you need it, it's good that it's available in the library somewhere IMO.

But still, even the Django doc states that they are not using the right prefixes, so...
willmcgugan: Thing is I'd rather not be descriptive regarding these units. There will always be developers who will want to do it the 'wrong' way to match some pre-existing convention perhaps.

Maybe the best approach would be to provide a way of using any unit system you like, but default to the more technically correct way.

BTW do you still favour the units in the PR or are you confused as I am? Django likes power of two for file sizes, OSX likes powers of 10.
althonos: Then let's name the functions after what they do, and not what standard they try to conform to: we rename `filesize.traditional` to `filesize.binary`. Then, we add a *kwarg* named `si` to `fs.binary` that defaults to `True`, to choose if we want the prefix or not.

```python
>>> fs.filesize.binary(4096)
4.0 KiB
>>> fs.filesize.binary(4096, si=False):
4.0 KB
>>> fs.filesize.decimal(4096)
4.1 kB
```

Here: `KiB` is the SI / ISO standard for binary, `kB` is the SI for decimal, and `KB` is the JDEC standard for binary.
willmcgugan: That sounds like a plan. How would you feel about leaving in `traditional` as it is at the moment, so that it wouldn't break `moya serve`?
althonos: * `fs.filesize.traditional(size)` will return `fs.filesize.binary(size, si=False)`.
* filesize functions will raise TypeError.
coveralls: 
[![Coverage Status](https://coveralls.io/builds/13583840/badge)](https://coveralls.io/builds/13583840)

Coverage increased (+4.0e-05%) to 99.949% when pulling **bf4f1accd73c55f3c8a79c81d32965fa4981c8ec on althonos:patch-1** into **7a274da5963f5dd77e9a953de7d0415d1df9f75e on PyFilesystem:master**.

coveralls: 
[![Coverage Status](https://coveralls.io/builds/13584782/badge)](https://coveralls.io/builds/13584782)

Coverage increased (+4.0e-05%) to 99.949% when pulling **75cd5a032ae4c544f8af489c7ba850e088ea2900 on althonos:patch-1** into **7a274da5963f5dd77e9a953de7d0415d1df9f75e on PyFilesystem:master**.

willmcgugan: Looks great. Just needs "decimal" in __all__. Happy to merge if you are?
althonos: I realize, if we are to keep traditionnal, then maybe we don't need the `si` kwarg, and just advertise everything in the documentation.

* `traditionnal` --> KB, MB, GB, .... /1024
* `binary` --> KiB, MiB, GiB, .... /1024
* `decimal` --> kB, MB, GB, .... /1000

(this looks like an endless debate :smile:)
coveralls: 
[![Coverage Status](https://coveralls.io/builds/13585761/badge)](https://coveralls.io/builds/13585761)

Coverage increased (+4.0e-05%) to 99.949% when pulling **05c960dd96b517186fc057754ab6c5d08e8fc9dd on althonos:patch-1** into **7a274da5963f5dd77e9a953de7d0415d1df9f75e on PyFilesystem:master**.

willmcgugan: The great thing about standards is they are so many to chose from...

Think you're right about the `si` kwarg. 
althonos: I squashed everything into a single commit, merge shoud be clear now. `fs.filesize` provides 3 functions, `binary`, `decimal`, and `traditional`.
willmcgugan: Great! I hope never to have to think about file size units again :)

Patch:
diff --git a/fs/filesize.py b/fs/filesize.py
index c019385..85c608d 100644
--- a/fs/filesize.py
+++ b/fs/filesize.py
@@ -9,7 +9,7 @@ Functions for reporting filesizes
 from __future__ import division
 from __future__ import unicode_literals
 
-__all__ = ['traditional', 'decimal']
+__all__ = ['traditional', 'decimal', 'binary']
 
 
 
@@ -17,7 +17,7 @@ def _to_str(size, suffixes, base):
     try:
         size = int(size)
     except ValueError:
-        raise ValueError(
+        raise TypeError(
             "filesize requires a numeric value, not {!r}".format(size)
         )
     if size == 1:
@@ -35,7 +35,7 @@ def _to_str(size, suffixes, base):
 def traditional(size):
     """
     Convert a filesize in to a string representation with traditional
-    (base 2) units.
+    (base 2) units and JDEC prefixes.
 
     :param int size: A file size.
     :returns: A string containing a abbreviated file size and units.
@@ -49,18 +49,35 @@ def traditional(size):
     )
 
 
+def binary(size):
+    """
+    Convert a filesize in to a string representation with binary units
+    and SI binary prefixes.
+
+    :param int size: A file size.
+    :param bool si: True to use SI prefixes, False to use JDEC prefixes.
+    :returns: A string containing a abbreviated file size and units.
+    :rtype str:
+
+    """
+    return _to_str(
+        size,
+        ('KiB', 'MiB', 'GiB', 'TiB', 'PiB', 'EiB', 'ZiB', 'YiB'),
+        1024
+    )
+
+
 def decimal(size):
     """
     Convert a filesize in to a string representation with decimal
-    units.
+    units and SI decimal prefixes.
 
     :param int size: A file size.
     :returns: A string containing a abbreviated file size and units.
     :rtype str:
     """
-
     return _to_str(
         size,
-        ('kbit', 'Mbit', 'Gbit', 'Tbit', 'Pbit', 'Ebit', 'Zbit', 'Ybit'),
+        ('kB', 'MB', 'GB', 'TB', 'PB', 'EB', 'ZB', 'YB'),
         1000
     )


Test patch:
diff --git a/tests/test_filesize.py b/tests/test_filesize.py
index 9bc2dcd..098fedc 100644
--- a/tests/test_filesize.py
+++ b/tests/test_filesize.py
@@ -41,6 +41,42 @@ class TestFilesize(unittest.TestCase):
             '1.5 MB'
         )
 
+    def test_binary(self):
+
+        self.assertEqual(
+            filesize.binary(0),
+            '0 bytes'
+        )
+        self.assertEqual(
+            filesize.binary(1),
+            '1 byte'
+        )
+        self.assertEqual(
+            filesize.binary(2),
+            '2 bytes'
+        )
+        self.assertEqual(
+            filesize.binary(1024),
+            '1.0 KiB'
+        )
+
+        self.assertEqual(
+            filesize.binary(1024 * 1024),
+            '1.0 MiB'
+        )
+
+        self.assertEqual(
+            filesize.binary(1024 * 1024 + 1),
+            '1.0 MiB'
+        )
+
+        self.assertEqual(
+            filesize.binary(1.5 * 1024 * 1024),
+            '1.5 MiB'
+        )
+
+
+
     def test_decimal(self):
 
         self.assertEqual(
@@ -57,25 +93,25 @@ class TestFilesize(unittest.TestCase):
         )
         self.assertEqual(
             filesize.decimal(1000),
-            '1.0 kbit'
+            '1.0 kB'
         )
 
         self.assertEqual(
             filesize.decimal(1000 * 1000),
-            '1.0 Mbit'
+            '1.0 MB'
         )
 
         self.assertEqual(
             filesize.decimal(1000 * 1000 + 1),
-            '1.0 Mbit'
+            '1.0 MB'
         )
 
         self.assertEqual(
             filesize.decimal(1200 * 1000),
-            '1.2 Mbit'
+            '1.2 MB'
         )
 
     def test_errors(self):
 
-        with self.assertRaises(ValueError):
-            filesize.traditional('foo')
\ No newline at end of file
+        with self.assertRaises(TypeError):
+            filesize.traditional('foo')