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

Shorthand "m" should be "min" #134

Closed
getsnoopy opened this issue Jun 14, 2020 · 13 comments
Closed

Shorthand "m" should be "min" #134

getsnoopy opened this issue Jun 14, 2020 · 13 comments

Comments

@getsnoopy
Copy link
Contributor

The "shorthand" m stands for metre, not minute (whose symbol is min). Using m is confusing, as not only is "metres" meaningless in the context of time, but if standards are not being followed, it can just as easily stand for "months" (whose conventional abbreviation is mo). The output from fmtShort for minutes should be changed to min. Also, there should be a space between the number and the unit symbol in the output from the function as per the SI.

@ktiy
Copy link

ktiy commented Jun 16, 2020

commonly used abbreviations for minutes include, 'm', 'min', and 'mins'
metres has no use in time, and shouldn't cause confusion

there's no standard 'shorthand', only a standard symbol (you're right, 'min'),

but i feel like 'm' might be most common, if not, really common

@getsnoopy
Copy link
Contributor Author

getsnoopy commented Jun 17, 2020

min is the most common because it's the SI standard. m might be used somewhat commonly, but it's incorrect because the SI doesn't allow abbreviations and it's ambiguous as I pointed out.

But my main point is that the context of time is only available to the developer using this library, not necessarily to the end user seeing its output. So if the output is 1m for example, the user has no way of knowing what that actually means. Actually, they do; it means "1 metre" as per the standard, but if it's being used incorrectly, then a user would have to reinterpret what it means in that context. 1 min, on the other hand, is unambiguous regardless of where it's displayed (mainly because it's a symbol and not an ad-hoc abbreviation).

@illuminist
Copy link

Context matters. Even it outputs a full unit, like 1 minute, but 1 minute of what?

Consider these 2 tables

------------------------
|  1 metre | 5 minutes |
| 4 metres | 8 minutes |
| 2 metres | 6 minutes |
------------------------

--Distance Traveled---Time Taken--
|               1 m |        5 m |
|               4 m |        8 m |
|               2 m |        6 m |
---------------------------------- 

You can see the unit abbreviation can derive the meaning nicely from its context.

@getsnoopy
Copy link
Contributor Author

@illuminist That assumes that every place that this module is used is attached to a label or the like, which isn't true. And again, the whole point of the SI is that context shouldn't determine the meaning of unit shorthand because it uses universal symbols.

In your example, if the table has 1000 rows instead of 3, and the headers are therefore off-screen, you have to try and remember, "Wait...is this 4 m in the actual sense of metres, or is this 4 m in the non-standard sense that this app is using where it can also mean minutes (or even months)? Let me scroll all the way up and find out." This situation is entirely avoided when standards are followed.

@styfle styfle changed the title Shorthand is incorrect Shorthand "m" should be "min" Dec 7, 2020
@leerob
Copy link
Member

leerob commented Mar 2, 2021

Closing per #136.

Edit: See discussion on that PR!

@PhiLhoSoft
Copy link

I agree with getsnoopy, as I was surprised by this abbreviation in Node output…
SI units, names and symbols, are strictly defined and typography codes follow them.
It would be unfortunate to stick to bad output because of a small mistake made at the start of the library… 😅

@leerob
Copy link
Member

leerob commented Jul 11, 2022

It's been over a year now, we can safely close this based on the lack of traction (x-ref #136).

Thank you regardless 🙏

@leerob leerob closed this as completed Jul 11, 2022
@getsnoopy
Copy link
Contributor Author

@leerob I'm not sure what you're looking for in terms of "traction". There's at least 5 people (including me) who want this to be changed at the moment, not to mention that it is the standards-compliant thing to do.

@leerob
Copy link
Member

leerob commented Jul 11, 2022

It would probably need to be >100, given this package is installed 152 million times per week and is one of the most used packages on npm. Feature additions should benefit the majority of the consumers of the package, and right now, ms is lean and serving it's purpose well – so just want to be extra cautious with adding new features that would increase the package size.

https://www.npmjs.com/package/ms

@getsnoopy
Copy link
Contributor Author

Well that's the thing—it's not a new feature; it's a bug fix. And the fix would increase the package size by 8 bytes, a trivial sum.

@ljharb
Copy link

ljharb commented Jul 11, 2022

@leerob it's a pretty shocking position to take that incorrectness and confusion are less damaging than adding a highly compressible two code units ("in") to bundle sizes.

@leerob
Copy link
Member

leerob commented Jul 11, 2022

There's not clear conviction this is a bug based on the conversation above and the lack of engagement from the community.

If that does end up changing and more folks find this behavior to be unintended we can revisit, but currently 6 upvotes on the original issue isn't enough to justify changing. The reason I mentioned package size is bceause 8 bytes is 1.2 GB @ 152 million downloads per week. Even small changes matter and should be driven by community feedback.

Thank you for your feedback and we can revisit this issue if it receives more traction. Specifically, please leave an upvote on the original issue.

@vercel vercel locked as resolved and limited conversation to collaborators Jul 11, 2022
@vercel vercel unlocked this conversation Jul 12, 2022
@leerob
Copy link
Member

leerob commented Jul 12, 2022

TIL you can't upvote locked issues - my bad!

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 a pull request may close this issue.

6 participants