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] [Chat Formatting] New rows to embeds util #4603

Open
wants to merge 4 commits into
base: V3/develop
Choose a base branch
from

Conversation

FixedThink
Copy link
Contributor

Type

  • Bugfix
  • Enhancement
  • New feature

Description of the changes

  • Added rows_to_embeds() util in chat_formatting.
  • Added two unit tests for the util.

Resolves #4600.

added rows_to_embeds util (in chat_formatting)
added two unit tests
@FixedThink FixedThink changed the title Rows embeds util [Util] [Chat Formatting] New rows to embeds util Nov 13, 2020
@FixedThink FixedThink changed the title [Util] [Chat Formatting] New rows to embeds util [Utils] [Chat Formatting] New rows to embeds util Nov 13, 2020
Copy link
Contributor

@Drapersniper Drapersniper left a comment

Choose a reason for hiding this comment

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

Just some things I've noticed at a glance ... I'm not a fan of the util name but can't think of an alternative either

- Renamed function from `rows_to_embeds` to `embeds_from_rows`
- Changed "type, optional" to "Optional[type]".
- Add embed field limit check (max = 25).
@FixedThink
Copy link
Contributor Author

Updated the files based on the comments above. I renamed the function to embeds_from_rows, maybe that sounds a bit better? At least the first word now makes clear what the function returns.

@Jackenmen Jackenmen added Category: Core - API - Utils Package This is related to stuff in `redbot.core.utils` Type: Feature New feature or request. labels Nov 16, 2020
@Jackenmen Jackenmen added this to the 3.4.4 milestone Nov 16, 2020
Copy link
Contributor

@bobloy bobloy left a comment

Choose a reason for hiding this comment

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

Alternative naming suggestion: embedify_list

@@ -284,7 +284,7 @@ def pagify(
yield in_text


def rows_to_embeds(
def embeds_from_rows(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def embeds_from_rows(
def embedify_list(

Alternative name option. Follows the naming scheme of pagify

@@ -203,7 +203,7 @@ def test_embed_list():
base_embed = Embed(title="Test")
rows = list(str(i) for i in range(11))

embed_list = chat_formatting.rows_to_embeds(
embed_list = chat_formatting.embeds_from_rows(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
embed_list = chat_formatting.embeds_from_rows(
embed_list = chat_formatting.embedify_list(

@@ -223,7 +223,7 @@ def test_embed_list_2():
base_embed = Embed(title="Test")
rows = list(str(i) for i in range(11))

embed_list = chat_formatting.rows_to_embeds(
embed_list = chat_formatting.embeds_from_rows(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
embed_list = chat_formatting.embeds_from_rows(
embed_list = chat_formatting.embedify_list(

@Jackenmen Jackenmen modified the milestones: 3.4.4, 3.4.5 Dec 15, 2020
@Jackenmen Jackenmen modified the milestones: 3.4.6, 3.4.7 Jan 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Core - API - Utils Package This is related to stuff in `redbot.core.utils` Type: Feature New feature or request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a util that generates a list of embeds based on a list of rows
5 participants