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

Add unit_separator to #humanize and #humanize_bytes #15176

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

CTC97
Copy link

@CTC97 CTC97 commented Nov 9, 2024

Adds a separate_unit parameter to the humanize and humanize_bytes methods. If selected adds a non-breaking space ('\u00A0') between the value and the unit.

Also includes relevant specs.

Fixes #14349

@CTC97 CTC97 changed the title `separate_unit for humanize and humanize_bytes separate_unit for humanize and humanize_bytes Nov 9, 2024
@straight-shoota
Copy link
Member

Maybe a configurable parameter could be better than a boolean flag?
This would give more flexibility and allow use cases where a different separator then a non-breaking space is needed.

@CTC97
Copy link
Author

CTC97 commented Nov 10, 2024

Maybe a configurable parameter could be better than a boolean flag?

Does it make sense to give the user full control over the separator, or should we define an enum that limits options?

@Sija
Copy link
Contributor

Sija commented Nov 10, 2024

Also, the default IMO should use a regular space for compatibility.

@straight-shoota
Copy link
Member

What would be the reason for limiting? I don't see why any arbitrary restriction would be useful. If someone want's a particular string as separator, let them have it.

@CTC97
Copy link
Author

CTC97 commented Nov 10, 2024

Just pushed the change to allow for custom unit separators. Thought about a Char but that felt overly restrictive as well. 👍

src/humanize.cr Outdated Show resolved Hide resolved
@straight-shoota straight-shoota changed the title separate_unit for humanize and humanize_bytes Add unit_separator to #humanize and #humanize_bytes Nov 11, 2024
Copy link
Contributor

@ysbaddaden ysbaddaden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a tiny nitpick: specs could use a Char whenever possible to demonstrate the encouraged use, use a String when there are more than one char, and just leave one check that we can pass a 1 char string (or nil, number or whatever).

@CTC97
Copy link
Author

CTC97 commented Nov 15, 2024

Just a tiny nitpick: specs could use a Char whenever possible to demonstrate the encouraged use, use a String when there are more than one char, and just leave one check that we can pass a 1 char string (or nil, number or whatever).

Good call. I cleaned up the tests and left the ones that I think are meaningful.

Copy link
Contributor

@ysbaddaden ysbaddaden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your patience while we ironed out this feature 🙇

@straight-shoota straight-shoota added this to the 1.15.0 milestone Nov 15, 2024
@CTC97
Copy link
Author

CTC97 commented Nov 15, 2024

Thank you for your patience while we ironed out this feature 🙇

Happy to! Honored to contribute. 🔮✨

@RX14
Copy link
Contributor

RX14 commented Nov 16, 2024

Should this parameter be added to the method documentation?

@straight-shoota straight-shoota removed this from the 1.15.0 milestone Nov 18, 2024
@@ -152,17 +152,17 @@ struct Number
# delimiter (see `#format`).
#
# See `Int#humanize_bytes` to format a file size.
def humanize(io : IO, precision = 3, separator = '.', delimiter = ',', *, base = 10 ** 3, significant = true, prefixes : Indexable = SI_PREFIXES) : Nil
humanize(io, precision, separator, delimiter, base: base, significant: significant) do |magnitude, _|
def humanize(io : IO, precision = 3, separator = '.', delimiter = ',', *, base = 10 ** 3, significant = true, unit_separator = nil, prefixes : Indexable = SI_PREFIXES) : Nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, documentation is needed.

We also have an opportunity to mention:

"Consider using '\u00A0' (non -breaking space) as the value for unit_separator, it can prevent the output from being split across lines".

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For sure - I'm not immediately seeing where I can add to the docs, can someone point me in the right direction? 😅

src/humanize.cr Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Number#humanize and Int#humanize_bytes should separate number and unit by a space
6 participants