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

Make humanize_list() use babel. #2982

Merged
merged 18 commits into from
Aug 3, 2020
Merged

Make humanize_list() use babel. #2982

merged 18 commits into from
Aug 3, 2020

Conversation

Drapersniper
Copy link
Contributor

Type

  • Bugfix
  • Enhancement
  • New feature

Description of the changes

humanize_list will now use babel.list.format_list for it's logic. This closes #2906

Drapersniper and others added 7 commits August 8, 2019 20:55
`[p]bankset maxbal` can be used to set the maximum bank balance

Signed-off-by: Guy <[email protected]>
Signed-off-by: guyre <[email protected]>
Signed-off-by: guyre <[email protected]>
Signed-off-by: guyre <[email protected]>
…babel_locale` function to get a valid locale based on bot locale.

Signed-off-by: guyre <[email protected]>
…babel_locale` function to get a valid locale based on bot locale.

Signed-off-by: guyre <[email protected]>
Signed-off-by: guyre <[email protected]>
@Flame442 Flame442 added the Type: Enhancement Something meant to enhance existing Red features. label Sep 5, 2019
Copy link
Contributor

@mikeshardmind mikeshardmind left a comment

Choose a reason for hiding this comment

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

This can't be merged till we officially handle #2850

Copy link
Contributor

@mikeshardmind mikeshardmind left a comment

Choose a reason for hiding this comment

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

This needs to include changes for places where humanize list is used to pass the current locale

@Drapersniper Drapersniper deleted the babel-list branch December 22, 2019 18:04
@Drapersniper Drapersniper restored the babel-list branch January 18, 2020 13:17
@Drapersniper Drapersniper reopened this Jan 18, 2020
@Drapersniper Drapersniper added the Category: Core - API - Utils Package This is related to stuff in `redbot.core.utils` label Jan 18, 2020
@Flame442 Flame442 added this to the 3.3.2 milestone Feb 22, 2020
@Jackenmen Jackenmen self-assigned this Feb 22, 2020
Copy link
Member

@Jackenmen Jackenmen left a comment

Choose a reason for hiding this comment

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

Docstring should contain some info about the style arg - what determines if locale supports specified style? Since it's not handled by Red, saying this is based on Babel's format_list should probably be enough unless you think it's worth saying more.

There is one other issue I see with this though - this will change the current behavior for the empty sequences and someone relying on this raising IndexError is gonna be surprised. I'm not sure if we want to pursue this breaking change in 3.4.0, but if we don't then you could add an if not items and raise IndexError there.

@Jackenmen Jackenmen removed this from the 3.3.2 milestone Feb 28, 2020
@Drapersniper
Copy link
Contributor Author

Whoever wants to pick this one up feel free to

@Jackenmen Jackenmen requested a review from palmtree5 as a code owner March 19, 2020 23:18
@Jackenmen Jackenmen added this to the 3.4.0 milestone Mar 19, 2020
@Jackenmen Jackenmen added the Breaking Change Will potentially break some cogs. label Mar 19, 2020
Drapersniper and others added 5 commits May 28, 2020 12:24
@Drapersniper Drapersniper added the QA: Passed Used by few QA members. Has been approved by the assigned QA member(s). label Jun 26, 2020
Copy link
Member

@Jackenmen Jackenmen left a comment

Choose a reason for hiding this comment

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

This PR is approved (approving again after conflict resolution), it is waiting for merge until we're ready for 3.4 release (hence the Blocked label).

@Jackenmen Jackenmen removed the Blocked label Aug 3, 2020
@Jackenmen Jackenmen merged commit 1a3e264 into Cog-Creators:V3/develop Aug 3, 2020
@Cog-CreatorsBot Cog-CreatorsBot added the Changelog Entry: Pending Changelog entry for this PR hasn't been added by repo maintainers yet. label Aug 3, 2020
@Jackenmen Jackenmen added Changelog Entry: Added Changelog entry for this PR has already been added to changelog PR. and removed Changelog Entry: Pending Changelog entry for this PR hasn't been added by repo maintainers yet. labels Aug 7, 2020
@Drapersniper Drapersniper deleted the babel-list branch August 18, 2020 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Will potentially break some cogs. Category: Core - API - Utils Package This is related to stuff in `redbot.core.utils` Changelog Entry: Added Changelog entry for this PR has already been added to changelog PR. QA: Passed Used by few QA members. Has been approved by the assigned QA member(s). Release Blocker This needs handling prior to the next non-hotfix release. Type: Enhancement Something meant to enhance existing Red features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No special case for 2-tuple humanize_list
5 participants