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

[Utils] adds humanize_number method to chat formatting #2836

Merged
merged 37 commits into from Aug 27, 2019

Conversation

@Drapersniper
Copy link
Contributor

commented Jul 4, 2019

Type

  • Bugfix
  • Enhancement
  • New feature

Description of the changes

This PR adds a new method to chat formatting that allows formatting int and float based on bot locale.

e.g. 1456425612 will now be shows as 1,456,425,612 on en_US

I've applied this method to all mentions of currency I came across (Audio, Bank, Economy, Trivia)

greis and others added 11 commits Jul 3, 2019
This allows developers to emojis to command messages
This is a modified version of tick()

It accepts True/False for Tick/Cross emoji and in addition to that any other emoji the bot can see
Applies humanize_int to all (hopefully) mentions of currencies
Adds humanize_int method to chat_formatting so that cog creators can use it as well
Applies humanize_int to all (hopefully) mentions of currencies
Adds humanize_int method to chat_formatting so that cog creators can use it as well
greis
Applies humanize_int to all (hopefully) mentions of currencies
Adds humanize_int method to chat_formatting so that cog creators can use it as well
greis and others added 4 commits Jul 4, 2019
It previously formatted it as "{int:,}"
…orks with the latter

Tested this and its working with
xx-XX
xx_XX
xx-XX.encoding

Which should work with all locales supported by the bot
Copy link
Member

left a comment

This looks great! Just one change being requested for now 😄

I'll also add that I think humanize_decimal might be a better name for the util, considering it works for any decimal number (int or float)

redbot/core/utils/chat_formatting.py Outdated Show resolved Hide resolved
Drapersniper added 2 commits Jul 6, 2019
Created `_get_babel_locale` which will now only be run on bot start up and locale change

`_get_babel_locale` will try to parse Red's locale to a value supported by `babel`
If it fails (`ValueError`) then it will try to get the closes match locale
If this fails (`ValueError`, `TypeError`, `babel.core.UnknownLocaleError`) then then the locale defaults to `en-US`
The babel locale is cached for faster calls going forward until Red's locale changes
… string locale to `babel.core.Locale` object

Is the locale is left none it uses Red's locale

Moved `get_babel_locale` and `_get_babel_locale` to to i18n

`humanize_int` now always uses the bots locale
@Drapersniper

This comment has been minimized.

Copy link
Contributor Author

commented Jul 6, 2019

Thanks @Tobotimus for the suggestions via DM made this a lot more useful!

I may or many not have only seen your mention of humanize_decimal its true, but decimal may confuse less familiar devs ? maybe humanize_number ? (however that kinda implies that string numbers also work, im not agaisn't trying to convert a number to int/float if i fails raise it back to user) i'll let you confirm the desired aproach here.

@skeith

This comment has been minimized.

Copy link

commented Jul 7, 2019

I'd say humanize_digit since it's actually called digit grouping separator.

@Tobotimus

This comment has been minimized.

Copy link
Member

commented Jul 7, 2019

I think humanize_number is best, I guess it's implied that the number will be formatted as a decimal :)

There shouldn't be any need to handle string inputs, simply state the argument type in the docs and users will figure it out.

humanize_digit is confusing to me, because a digit refers to a single character in a number

@Drapersniper Drapersniper changed the title [Utils] adds humanize_int method to chat formatting [Utils] adds humanize_number method to chat formatting Jul 10, 2019
Signed-off-by: Draper <guyreis96@gmail.com>
Signed-off-by: Draper <guyreis96@gmail.com>
Copy link
Member

left a comment

Just a few small changes for now :)

changelog.d/2836.feature.rst Outdated Show resolved Hide resolved
changelog.d/2836.misc.rst Outdated Show resolved Hide resolved
redbot/cogs/cleanup/cleanup.py Outdated Show resolved Hide resolved
redbot/cogs/cleanup/cleanup.py Outdated Show resolved Hide resolved
redbot/cogs/cleanup/cleanup.py Outdated Show resolved Hide resolved
redbot/cogs/cleanup/cleanup.py Outdated Show resolved Hide resolved
redbot/cogs/economy/economy.py Show resolved Hide resolved
redbot/cogs/economy/economy.py Show resolved Hide resolved
redbot/cogs/economy/economy.py Outdated Show resolved Hide resolved
redbot/core/modlog.py Outdated Show resolved Hide resolved
Signed-off-by: Draper <guyreis96@gmail.com>
Copy link
Contributor Author

left a comment

I've also added the Override to en_US on a few errors and and log messages that you didn't mention on the review, let me know if you'd prefer reverting it.

redbot/core/bank.py Show resolved Hide resolved
redbot/core/bank.py Show resolved Hide resolved
redbot/core/bank.py Show resolved Hide resolved
redbot/core/bank.py Show resolved Hide resolved
@Drapersniper Drapersniper referenced this pull request Aug 26, 2019
3 of 3 tasks complete
redbot/core/i18n.py Show resolved Hide resolved
Drapersniper and others added 4 commits Aug 27, 2019
Signed-off-by: guyre <27962761+drapersniper@users.noreply.github.com>
Signed-off-by: guyre <27962761+drapersniper@users.noreply.github.com>
# Conflicts:
#	redbot/cogs/economy/economy.py
@Tobotimus Tobotimus merged commit 3c1b6ae into Cog-Creators:V3/develop Aug 27, 2019
1 check passed
1 check passed
Travis CI - Pull Request Build Passed
Details
@Drapersniper Drapersniper deleted the Drapersniper:humanize_int branch Aug 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.