-
Notifications
You must be signed in to change notification settings - Fork 270
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
Fix display of time units as per the SI #14
base: master
Are you sure you want to change the base?
Conversation
The SI requires symbols (not abbreviations) for units and spaces between quantities and their unit symbols. This commit fixes these issues in the time formatter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello, 👋 Thanks for your contribution. 👍
I think we're fine with "scientist" compressed unit style. We can see the same in various other libraries :
- https://github.com/briannesbitt/Carbon/blob/master/src/Carbon/Lang/en.php
- http://unicode.org/Public/cldr/39/
Sorry for the late feedback. 🙏
@kylekatarnls The SI is the "scientist unit style". And it's actually not a matter of style; it's the only proper way to write units. I'm not sure about Carbon (I'll submit a pull request there as well), but as for CLDR, they had it incorrectly in their previous library versions. I'm already working with them to fix it in the next version of CLDR. |
Oh, that would be a wide change for CLDR as it is driving indirectly a lot of other libraries and I think it's there for years, maybe ProtonVPN also took it from there. And sure if it changes there, we should really consider to change too. Good to know the space matters. Thanks for the tip. I would still make myself advocate for the devil, we are here in a context where there is no ambiguity about the meaning in good faith (it will appear next to "duration"). So probably there is to be considered the readability vs. SI-compliance and balance it, maybe if we go for a space before and after we should also add a comma between values. |
I also checked other clients (such as the Android VPN app) and it sounds we're doing the same (compressed) there. So we should probably take a decision at product level to apply to all. |
Yes, it is sort of the de facto internationalization library, so it is unfortunate that the mistake was in there. But it is going to be fixed, so it should be fixed here as well.
Being an advocate for the devil does not sound like a glamorous career :) But while the label might be there, the advantage with the SI is that context never matters or is needed.
While the comma is a style issue, I'd argue it's unnecessary. Any mixed units in the SI (or otherwise) are written with just spaces; for example, heights in US customary units or imperial units are written as |
Readability issue was not about
And so that's why I was proposing to work around with the coma. This problem actually does not exist with |
The SI requires symbols (not abbreviations) for units and spaces
between quantities and their unit symbols. This commit fixes these
issues in the time formatter.