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

feat: ExtendedEmbed with some convenience methods and removed pytz #72

Merged
merged 1 commit into from
Mar 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,6 @@ repos:
asyncpg-stubs,
polyfactory,
discord-py,
types-pytz,
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (llm): The removal of types-pytz from the pre-commit configuration suggests a move away from the pytz library. This change should ideally be accompanied by tests to ensure that any datetime-related functionality in the project, especially those involving timezones, continues to work as expected with the new setup. Please ensure that existing tests cover this aspect well, or consider adding new tests if necessary.

]
- repo: https://github.com/sphinx-contrib/sphinx-lint
rev: "v0.9.1"
Expand Down
12 changes: 1 addition & 11 deletions pdm.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ dependencies = [
"alembic>=1.13.0",
"pre-commit>=3.6.2",
"ruff>=0.1.7",
"pytz>=2024.1",
]
requires-python = ">=3.11,<4.0"
readme = "README.md"
Expand Down
5 changes: 2 additions & 3 deletions src/byte/lib/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,12 @@
import json
import re
import subprocess
from datetime import datetime
from datetime import UTC, datetime
from enum import StrEnum
from itertools import islice
from typing import TYPE_CHECKING, TypedDict, TypeVar

import httpx
import pytz
from anyio import run_process
from discord.ext import commands
from ruff.__main__ import find_ruff_bin # type: ignore[import-untyped]
Expand Down Expand Up @@ -414,7 +413,7 @@ async def query_all_peps() -> list[PEP]:
"status": PEPStatus(pep_info["status"]),
"type": PEPType(pep_info["type"]),
"topic": pep_info.get("topic", ""),
"created": datetime.strptime(pep_info["created"], "%d-%b-%Y").replace(tzinfo=pytz.utc),
"created": datetime.strptime(pep_info["created"], "%d-%b-%Y").replace(tzinfo=UTC),
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (llm): The change from pytz.utc to UTC in the query_all_peps function seems to be a part of a larger refactoring effort. However, it's essential to ensure that this change does not affect the functionality of any components relying on the query_all_peps function. Could you please add or update unit tests to verify that the datetime objects returned by query_all_peps are still correctly timezone-aware and behave as expected in time calculations and comparisons?

"python_version": pep_info.get("python_version"),
"post_history": pep_info.get("post_history", []),
"resolution": pep_info.get("resolution"),
Expand Down
41 changes: 18 additions & 23 deletions src/byte/plugins/python.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
from byte.lib.common.assets import python_logo
from byte.lib.common.colors import python_blue, python_yellow
from byte.lib.utils import PEP, query_all_peps
from byte.views.embed import ExtendedEmbed, Field
from byte.views.python import PEPView

__all__ = ("Python", "setup")
Expand All @@ -29,10 +30,10 @@ async def _pep_autocomplete(self, interaction: Interaction, current_pep: str) ->
.. warning:: ``interaction`` is not used, but is required.
"""
return [
Choice(name=f'PEP {number} - {pep["title"]}', value=str(number))
for number, pep in self._peps.items()
if current_pep.lower() in str(number) or current_pep.lower() in pep["title"].lower()
][:25]
Choice(name=f'PEP {number} - {pep["title"]}', value=str(number))
for number, pep in self._peps.items()
if current_pep.lower() in str(number) or current_pep.lower() in pep["title"].lower()
][:25]

@app_command(name="pep")
@autocomplete(pep=_pep_autocomplete)
Expand All @@ -52,26 +53,20 @@ async def peps(self, interaction: Interaction, pep: int) -> None:

docs_field = f"- [PEP Documentation]({pep_details['url']})\n"

minified_embed = Embed(title=f"PEP #{pep_details['number']}", color=python_blue)
minified_embed.add_field(name="Summary", value=pep_details["title"], inline=False)
minified_embed.add_field(name="Status", value=pep_details["status"], inline=True)
minified_embed.add_field(name="Type", value=pep_details["type"], inline=True)
minified_embed.add_field(name="Authors", value=", ".join(pep_details["authors"]), inline=False)
minified_embed.add_field(name="Created", value=pep_details["created"], inline=True)
minified_embed.add_field(name="Python Version", value=pep_details["python_version"], inline=True)
minified_embed.add_field(name="Documentation", value=docs_field, inline=False)
fields = [
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (llm): The refactoring to use ExtendedEmbed and Field in python.py is a significant change. While this makes the code cleaner, we need to ensure it doesn't introduce regressions. Please add integration tests that verify the peps command still produces the correct embed structure, especially focusing on the correct rendering of fields and the embed's thumbnail. It would also be beneficial to test how the command handles missing or incomplete data for a PEP.

Field(name="Summary", value=pep_details["title"], inline=False),
Field(name="Status", value=pep_details["status"], inline=True),
Field(name="Type", value=pep_details["type"], inline=True),
Field(name="Authors", value=", ".join(pep_details["authors"]), inline=False),
Field(name="Created", value=str(pep_details["created"]), inline=True),
Field(name="Python Version", value=pep_details["python_version"], inline=True),
Field(name="Documentation", value=docs_field, inline=False),
]
minified_embed = ExtendedEmbed.from_field_dicts(
title=f"PEP #{pep_details['number']}", color=python_blue, fields=fields
)
minified_embed.set_thumbnail(url=python_logo)

embed = Embed(title=f"PEP #{pep_details['number']}", color=python_blue)
embed.add_field(name="Summary", value=pep_details["title"], inline=False)
embed.add_field(name="Status", value=pep_details["status"], inline=True)
embed.add_field(name="Type", value=pep_details["type"], inline=True)
embed.add_field(name="Authors", value=", ".join(pep_details["authors"]), inline=False)
embed.add_field(name="Created", value=pep_details["created"], inline=True)
embed.add_field(name="Python Version", value=pep_details["python_version"], inline=True)
embed.add_field(name="Documentation", value=docs_field, inline=False)
embed.set_thumbnail(url=python_logo)

embed = minified_embed.deepcopy()
view = PEPView(author=interaction.user.id, bot=self.bot, original_embed=embed, minified_embed=minified_embed)
await interaction.followup.send(embed=minified_embed, view=view)

Expand Down
56 changes: 56 additions & 0 deletions src/byte/views/embed.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
from copy import deepcopy
Copy link
Contributor

Choose a reason for hiding this comment

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

issue (llm): I noticed that the new ExtendedEmbed class and related functionality are introduced in this pull request. However, I couldn't find any unit tests covering these new additions. It's crucial to ensure that the new methods (add_field_dict, add_field_dicts, from_field_dicts, and the deepcopy method) work as expected under various conditions. Could you please add unit tests for these methods, including tests for edge cases such as empty fields list, invalid field parameters, and ensuring deepcopy indeed creates a deep copy?

from datetime import datetime
from typing import Any, Literal, Self, TypedDict

from discord import Colour, Embed

__all__ = ("ExtendedEmbed", "Field")


# TODO: find a better place


class Field(TypedDict):
# NOTE: types are matching the ones in ``Embed`.add_fields`
name: Any
value: Any
inline: bool = True


class ExtendedEmbed(Embed):
# TODO: better name
def add_field_dict(self, field: Field) -> Self:
self.add_field(**field)
return self

def add_field_dicts(self, fields: list[Field]) -> Self:
for field in fields:
self.add_field_dict(field)
return self

@classmethod
def from_field_dicts(
cls,
colour: int | Colour | None = None,
color: int | Colour | None = None,
title: Any | None = None,
type: Literal["rich", "image", "video", "gifv", "article", "link"] = "rich",
url: Any | None = None,
description: Any | None = None,
timestamp: datetime | None = None,
fields: list[Field] | None = None,
) -> Self:
embed = cls(
colour=colour,
color=color,
title=title,
type=type,
url=url,
description=description,
timestamp=timestamp,
)
embed.add_field_dicts(fields or [])
return embed

def deepcopy(self) -> Self:
return deepcopy(self)
Loading