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

Input Upper Case characters = error #14

Closed
joelabair opened this issue May 20, 2014 · 8 comments
Closed

Input Upper Case characters = error #14

joelabair opened this issue May 20, 2014 · 8 comments

Comments

@joelabair
Copy link

bash >node

bytes = require('bytes')
[Function]
bytes('5MB')
TypeError: Cannot read property '1' of null
at module.exports (./node_modules/bytes/index.js:13:27)
at repl:1:2
at REPLServer.self.eval (repl.js:110:21)
at Interface. (repl.js:239:12)
at Interface.EventEmitter.emit (events.js:95:17)
at Interface._onLine (readline.js:202:10)
at Interface._line (readline.js:531:8)
at Interface._ttyWrite (readline.js:760:14)
at ReadStream.onkeypress (readline.js:99:10)
at ReadStream.EventEmitter.emit (events.js:98:17)
bytes('5mb')
5242880

@benmosher
Copy link

+1

1 similar comment
@theofidry
Copy link
Contributor

+1

@theofidry
Copy link
Contributor

Fixed in my fork https://github.com/theofidry/bytes.js, waiting for merge.

@theofidry theofidry mentioned this issue Mar 16, 2015
@rhys-vdw
Copy link

I'm confused by this, isn't Mb megabits, whereas MB is megabytes? It seems the API of this module is kind of broken in general. Shouldn't mb be an error, as lower-case m has no meaning in this context?

@tj
Copy link
Member

tj commented Mar 30, 2015

@rhys-vdw technically MiB no? This module was never really intended to get that specific, but I'd +1 a new major-level release that fixes things up. Personally I'd still prefer --limit 5mb over --limit 5MiB etc but hey, maybe we can just leave that as the default behaviour that we see now

@theofidry
Copy link
Contributor

According to Wikipedia, it's MB.

As for using mb as an input, I would be more the kind to be fault tolerant and interpret it as MB instead of throwing an error.

But that raises the point of what will happen if you pass a value not understood (like 1my), and written in the Readme, it returns null.

@rhys-vdw
Copy link

I think @tj is correct in that the units that this library deals with are multiples of 1024, so technically 'mb' is interpreted as 'mibibytes'. However, in practise I never hear anyone using that term. I think I'd refer to mibibytes as 'megabytes'.

If it's interesting, the reason I suggested this was that I wanted to store the string in a config file, and also forward the error to the user. So:

maxFileSize = config.get('maxFileSize');
if (file.size > bytes(maxFileSize)) {
  throw new Error('File exceeds maximum size of ' + maxFileSize);
}

But I don't want my error message (or config) to have the incorrect units. Obviously there are numerous workarounds for this (simplest being to change cases contextually).

I see now that this is really a pretty simple module with limited use cases. Unless you want to expand to handling conversion between different units (bytes, bits, mibibytes, megabytes) @theofidry's solution is probably most appropriate.

@tj
Copy link
Member

tj commented Apr 12, 2015

I wouldn't mind support for bits/bytes since network config is typical in bits, but I'd definitely worry about people misconfiguring with different units

@tj tj closed this as completed Apr 12, 2015
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

No branches or pull requests

5 participants