Luminous Lightyears #4
Conversation
* basic bot booting * leaving the game instance * Ping Player on Joining, Leaving, Starting the game * Convert UI to embeds * Removing testing code * Converting game_start to an extension of main.py * Changing lib to src * Removed useless double starting of the bot
* Handle if token isn't provided * Add logging * make python-dotenv optional * Add developer mode * Add periods to docstrings
* Handle if token isn't provided * Add logging * make python-dotenv optional * Add developer mode * Add periods to docstrings * Templating draft * fix typo * fix typo
* Add embedding of Template * Linting * Linting check 2 * Linting * Linting ;-; * Improve docstrings and simplify condition converter * linting --------- Co-authored-by: hazyfossa <[email protected]>
* Basic game management draft * Implement random game id's * Player registration UI prototype * Get nation name from user for registration * Refactor how players are added to a game instance * Remove message when registering * Revert to sending reply for modal response * Updating this models from main * Changing class State to the name PlayerState * Change the name to GameInteraction --------- Co-authored-by: Maheshkumar <[email protected]> Co-authored-by: Sapient44 <[email protected]>
* Send stats during stage switches * Fix and format templates
* Add embeds to interaction.py * Update game_interaction.py Changed similar error detecting ifs to elif * Apply formatting --------- Co-authored-by: Maheshkumar <[email protected]>
* Init doc * Add first draft * Add install section * Changed a few mistakes 1) Capitalisation of about \n 2) Removal of present time info enchancement * Add basic demo for the game * Add contributions * Add link of contributions * Table of contents * Update README.md * Update README.md * Update README.md * Update README.md * Convert attributes to table * Update README.md * Update README.md * Update README.md * Update README.md * Update README.md --------- Co-authored-by: Sapient44 <[email protected]> Co-authored-by: Sapient44 <[email protected]>
There was a problem hiding this comment.
Commit Quality
Commits are fairly lackluster. The commit titles are mostly ok, however I did not find any commits with commit bodies
and most commits generally lacked any description or context that would make the commit useful. These types of
"title-only" commits can be acceptable for fairly small changes where the commit title and change itself are
very self-explanatory. Most of the commits on this project were more involved and definitely needed more info
added to the commit title and/or body. I.e for a commit that says something like fix y, it is not just important
to say what you did, but also why you did it. It is important to be able to look back on the codebase and
understand why changes are made. instead of just writing something like fix help command, I would much prefer
if a commit message was more similar to:
fix bug in help command
The help command was using the incorrect color on the embed. I have changed it to use the system default color
so that this command matches the rest of the application.
This gives important context that may be missed, forgotten, or questioned at a later time/date. It is also important
at code review time to understand why changes were made. With my above example, the reason is a bit obvious, but for
more complex code changes, this added context can be very helpful. It also cements the reasoning as part of the codebase.
I have seen many error regressions in codebases because a line of code was changed which reintroduced a bug that was patched
long ago by a fix with an unclear commit message.
Code Quality
Overall the code quality is quite good. There were some quircks and oddities, but this is mostly on-par with what I would
expect in a professional codebase. Most of the logic is easy to follow and code comments are sparse, but used appropriately.
In my opinion, this is exactly how they should be used.
Documentation
The readme is very good. Instructions are clear, I was able to set up the project quickly and the overview of
functionality is very concise. I really appreciate the known issues as well as documentation for disabling actor
thumbnails to help resolve a known issue. This very good documentation and excellent use of environment variables.
Generally docstrings were ok. I would like to see more descriptive docstrings that help understand the function and
purpose of classes, functions, and methods. I feel that many docstring were only there to satisfy the linter. In
most cases that was fine since the logic was clear and did not need much additional explanation.
Structure
Overall the structure of the codebase is quite nice. I appreciate that code is organized in a src directory. It is
also good that characters is its own module, there is a lot of logic here and it could cause clutter.
I could see dividing the code further into smaller modules, i.e the game logic versus the character/player logic
be put in their own modules, though the size of this project means that this is not really necessary.
I do believe the characters data could be better saved in some other format than as Python. It could improve project
strucutre and make configuration easier if all characters were stored in a more user-friendly strucutre than Python.
Writing the configurations in python also required disabling of auto-formating as well as ignoring linting rules,
which should generally hint that there may be a better way to go about things.
Formatting
Code formatting was overall very good. There were a few places where linting was ignored with purpose and at the end
of the day, those things are fine to be decided/agreed upon for a team. I would like to see linting followed more
strictly since that is the reason it exists. If rules are going to be frequently ignored, the rule itself should
usually be altered, or perhaps the rule should not be broken in the first place.
The Complete Picture
I was impressed with the use of type generics and protocol typing for WeightedList. I do not often see this used
and I really appreciate when it is done well to add clarity to a codebase. Code styling was very good as well. I
believe rules were bent a bit too often, but overall code was clean, concise, and easy to navigate.
The use of Templates and Python to configure characters was an interesting and creative choice. Although I commented
that a file structure such as yaml might be better suited to the current use of characters in this project, I could
see the use of Python as also being a large benefit. This could make character configuration much more flexible and
expandable in the future, which is certainly a plus. Definitely no "points" lost to a creative decision such as this.
I would like to see some form of testing on a repo like this. Game logic can quickly get complex and have many side-effects.
Simple unit testing can go far to ensure code qualtiy. (Though I understand that testing may be ignored due to time-constraints).
| """Alex the analyst media templates and information.""" | ||
|
|
||
| from src.templating import Actor, StageGroup | ||
| from src.templating import ChoiceTemplate as t # noqa: N813 |
There was a problem hiding this comment.
Single letter variables like this are not very Pythonic. t does not make it entirely clear what this variable name is representing. I would not generally recommend ignoring linting rules like this since the single-name variable reduces clarity in the codebase.
This could be considered a bit of a nitpick, since in other languages, such as Golang, this type of variable naming is acceptable. Still t is not a clear representation of ChoiceTemplate. At the very least, I would recommend clearer naming such as ct, which is similar to something like import numpy as np. Though I would prefer to just use the original name of the import unless there is a name conflict (i.e imporing Template from multiple modules, it may become necessary to from characters import Template as character_template and from other_module import Template as other_template
| error_color = (255, 58, 51) | ||
| message_color = (124, 199, 242) | ||
| system_message_color = (88, 245, 149) |
There was a problem hiding this comment.
All constants should use SCREAMING_SNAKE_CASE, using mixed lowercase and uppercase snake case here implies that these variables could be dynamic, which conflicts with the naming of the file.
| from dotenv import load_dotenv | ||
|
|
||
| load_dotenv() | ||
| no_dotenv = False |
There was a problem hiding this comment.
Double negative variable naming could be a bit confusing. I.e no_dotenv = True means a dotenv was not used. I would prefer something like dotenv_used = False. So that you can use the expression if not dot_env used:, which reads a bit clearer when reading through the logic.
| if no_dotenv: | ||
| logger.info("To read token from .env, install 'python-dotenv'") | ||
|
|
||
| exit() |
There was a problem hiding this comment.
Exiting without an error code when an error occurred is generally not good practice. Using exit() this way could indicate to a user that the application completed successfully. It would be more explicit to raise an exception here since the application failed to startup and is completely unable to run without a bot token.
Furthermore, if the application could continue running normally after this exception occurs, it could be appropriate to use logging.exception to log exception info along with the regular log error. (i.e imagine a web server where an error occurs for a request, but you would like the web server to continue serving other users, it is appropriate to log the exception, rather than exiting the program.)
| def get_developer_mode() -> bool: | ||
| """Get if the bot is running in dev mode.""" | ||
| try: | ||
| return argv[1] == "--dev" |
There was a problem hiding this comment.
A better way to do this would be to use argparse for parsing arguments. At the very least, a flag like this could be parsed simply with return "--dev" in argv, this would save the exception handling and would allow the user to pass the flag anywhere in the command, rather than a specific position. This is especially helpful if you are allowing multiple flags, since it may not be user friendly to require flags to be given in a specific order.
| await player.ctx.send( | ||
| embed=Embed( | ||
| title="Some error occured", | ||
| description=f"{e} \n has occured, please contact the devs if you see this", |
There was a problem hiding this comment.
Displaying a python exception for debug purposes can be very helpful, but errors like this should not be rendered directly to a user in a production environment.
| return | ||
|
|
||
| character = all_characters.get_random(player.state) | ||
| for attr in self.values_to_check: |
There was a problem hiding this comment.
It would be clearer if the values_to_check were defined here. It is also unclear what these values are being checked for exactly. This could perhaps use more documentation, be broken up into a function, or otherwise made to be more clear.
| class SupportedRandomValue(Protocol): | ||
| """Generic type for supported classes.""" | ||
|
|
||
| @property | ||
| def weight(self) -> int: ... # noqa: D102 | ||
|
|
||
| def is_available(self, state: "PlayerState") -> bool: ... # noqa: D102 | ||
|
|
||
|
|
||
| T = TypeVar("T", bound=SupportedRandomValue) | ||
|
|
||
|
|
||
| class WeightedList(Generic[T]): |
There was a problem hiding this comment.
I really like the use of type generics + protocol typing here. I am not too familiar with this; however it is a good use case and offers utility in my IDE that adds clarity to the codebase.
| sleep_time = 6 + (random.uniform(-1, 0.75)) | ||
|
|
||
| character = all_characters.get_random(player.state) | ||
| while self.stage not in character.stages: |
There was a problem hiding this comment.
As far as I can tell there is no guarantee that this while loop will ever complete. I.e if there is a single character that causes this loop to repeat, it could be called indefinitely until the heat death of the universe. Similar logic is implemented in the get_random method.
Instead, it would be preferable to first build a list of valid choices and select from those valid options, instead of trying forever to find a valid option.
This code also begs the question "what if there are no valid selections remaining?" Would it be desirable to repeat forever if that were to happen?
| while result is None: | ||
| possible_result = random.choices(self.values, weights=self.weights, k=1)[0] | ||
|
|
||
| if possible_result.is_available(state): | ||
| result = possible_result |
There was a problem hiding this comment.
Similar to a previous comment: this seems like it could repeat forever if the same choice were made multiple times. Furthermore, would it be desirable to repeat forever if there were no valid options?
No description provided.