Contemplative Constellations #2
Conversation
Co-authored-by: Paillat <[email protected]>
Co-authored-by: Paillat <[email protected]>
Co-authored-by: Paillat <[email protected]>
Add basic code for the bot
Tvdb UI classes rework + profile command + view error handling
Prevent interaction with views by others
Update readme
Various changes to presentation
📝 Add presentation and images
There was a problem hiding this comment.
README
The README is great from a developer point of view. I would include more information from your presentation itself to describe what the project does. It's not immediately obvious that you have a presentation without a direct link, so I think either duplicating some of the content in the README itself or linking to the presentation directly can go a long way.
The presentation is solid. It explains and showcases the features nicely.
Commit Quality
Commits seem solid as a whole with good name and convention descriptions. Some commits could be improved in general ("clean up some code" or "fix"), but that's always the case with bigger projects during a time crunch.
Code Quality
The code overall is very well written and easy to follow. There are some cases where I think the code is rather dense and could use a combination of linebreaks and more descriptive docstrings. There are other cases where the comments and additional documentation provides a lot of context as to some decisions made in the code, so that was great to see.
I think the utils/ folder is a bit unnecessarily saturated and some functions could be re-located closer to where they are being used.
The care taken to implement rate limits and caching so as not to stress the API is very appreciated. The models used to represent the different API results is well done as well. The discord library is also handled well. There is one instance, when displaying favorite in the profile, where a lack of truncating or pagination could cause trouble. So I would be weary there and a few other places that end up truncating without informing the end user.
The Complete Picture
The project concept is a simple on: interact with the TVDB api to help users manage and showcase which TV shows and media they like directly from a Discord server. It fulfills our theme and tech requirement. The code implementation shows a great deal of care and thought. It's also easy to read and mostly easy to navigate as well, if you're familiar with the additional libraries being used.
It would have been nice to see a few more features supported beyond searching, favoriting, and viewing profiles. But I understand we did ask you to complete this project in 10 days from the ground up--so there's only so much you can do.
Overall, very nicely done! I really enjoyed reviewing this project and I could see this being very useful in discord servers that like to chat about TV shows and movies.
| self.db_session = db_session | ||
| self.cache = cache | ||
|
|
||
| self.event(self.on_ready) |
There was a problem hiding this comment.
A slightly unusual choice to do this rather than use the typical decorator.
| type JSON_DATA = dict[str, JSON_DATA] | list[JSON_DATA] | str | int | float | bool | None # noice | ||
|
|
||
| type SeriesRecord = SeriesBaseRecord | SeriesExtendedRecord | ||
| type MovieRecord = MovieBaseRecord | MovieExtendedRecord | ||
| type AnyRecord = SeriesRecord | MovieRecord |
There was a problem hiding this comment.
Nice usage of the new type statement to create type aliases
| class FetchMeta(Enum): | ||
| """When calling fetch with extended=True, this is used if we want to fetch translations or episodes as well.""" | ||
|
|
||
| TRANSLATIONS = "translations" | ||
| EPISODES = "episodes" |
There was a problem hiding this comment.
Looking at your usage of this later, you could benefit from using StrEnum here.
| if data is None: | ||
| raise ValueError("Data can't be None but is allowed to because of the broken pydantic generated models.") |
There was a problem hiding this comment.
This probably needs a more in-depth comment to explain what's going on here. It's kind of clear what's going on, but a more detailed commented for future-you/other developers would help.
| ) -> Self: ... | ||
|
|
||
| @classmethod | ||
| async def fetch( |
There was a problem hiding this comment.
This function could use a bit more information as to what it does, either in the docstring or as comments in the body. It wouldn't be immediately clear to me that it checks the cache in this function based on the docstring and the name.
The code is fairly efficient but also dense with no line breaks between different if-blocks. It can take someone who is new to the codebase a bit longer to parse out exactly what's happening where and why.
| ) | ||
| embed.add_field(name="Stats", value=stats_str, inline=False) | ||
|
|
||
| # TODO: What if there's too many things here, we might need to paginate this. |
There was a problem hiding this comment.
It seems for favorite you don't truncate like you do for the selections. I also don't see any checking for going over the limit of what can be contained in an embed. That case is worth checking for and, at minimum, truncating if you're in danger of going over the embed limit or truncating after a certain amount of favorites.
| async def get_cat_image_url(http_session: aiohttp.ClientSession) -> str: | ||
| """Get a URL for a random cat image. | ||
|
|
||
| The produced image can also be a GIF. | ||
| """ | ||
| async with http_session.get(CAT_API_URL) as resp: | ||
| resp.raise_for_status() | ||
| data = await resp.json() | ||
| return data[0]["url"] |
There was a problem hiding this comment.
Two things here:
- I love that you're using the cat api to pull a random cat image/GIF
- As far as I can tell this is only used in the help embed. I'd rather pull this function into that cog rather than put it in the generic utils folder, where it's unclear what its purpose is.
In general I prefer the utils/ folders to be true utilities that are actively used by multiple places in a system. I don't think it's worth pre-emptively putting non-generic utils in a utils/ folder in case you might use it in a different place later.
| get_logger(logger_name).setLevel(TRACE_LEVEL) | ||
|
|
||
|
|
||
| def _setup_external_log_levels(root_log: LoggerClass) -> None: |
There was a problem hiding this comment.
If you don't use root_log at all, do you need it? You always do get_logger() in the function itself.
| def mention_command(command: discord.ApplicationCommand[Any, ..., Any]) -> str: | ||
| """Mentions the command using discord markdown.""" | ||
| return f"</{command.qualified_name}:{command.qualified_id}>" |
There was a problem hiding this comment.
Only used in the help command, I would just relocate it to that file since this isn't very general in terms of usage.
| def by_season(episodes: list["Episode"]) -> dict[int, list["Episode"]]: | ||
| """Group episodes by season.""" | ||
| seasons = {} | ||
| for episode in episodes: | ||
| seasons.setdefault(episode.season_number, []).append(episode) | ||
| return seasons |
There was a problem hiding this comment.
Similar to the other comments I've left, this seems hyper-specific to Episodes. Why put it in utils/?
No description provided.