Mesmerizing Meteors #5
Conversation
Added the bot directory
-> /ping -> /pages commands added
Added database.py
Added the database and modified database.py
fix some linting issues make adjustments in 'paginator.py' add usage of dotenv
Modify database
Fixed database.py
Update database.py
Fixed linting issues on database.py
Fix database
Fixed remaining issue in database
Removed the Quiz table
Clean up unused commands / testing commands
Previously unclear that you had to use print to see results in code playground. The playground now also runs pre_code, so variables will be set in playground as well
…_auto Improve code playground messages
Co-authored-by: Gustav Odinger <65498475+gustavwilliam@users.noreply.github.com>
Since the feedback for losing lives on a level is minimal, give the user some info about how lives work before lvl 1, so all players know to look in the top right corner
Added special golfing levels
Add notice about lives on level 1
Co-authored-by: Gustav Odinger <65498475+gustavwilliam@users.noreply.github.com>
Co-authored-by: Gustav Odinger <65498475+gustavwilliam@users.noreply.github.com>
Co-authored-by: Gustav Odinger <65498475+gustavwilliam@users.noreply.github.com>
Co-authored-by: Gustav Odinger <65498475+gustavwilliam@users.noreply.github.com>
- Old cloning link doesn't work
MarkKoz
left a comment
MarkKoz
left a comment
There was a problem hiding this comment.
Hi Mesmerizing Meteors, fantastic work with your project. I commend you on making a complete and functional project! You effectively limited your scope, and did a great job making design decisions and compromises to match your circumstances. You even went above and beyond in some areas, like documentation.
The comments I left are from a perspective of having more time and resources to do things. I recognise that some of the suggestions I made were not realistic for your time frame, skill level, etc. My goal was to help you learn some new things that you can apply in the future.
I think the main growth area is the design of your persistence/data layer.
| @@ -0,0 +1 @@ | |||
| Folder Containing the project itself | |||
There was a problem hiding this comment.
Nit: this README doesn't seem to add much value nor can I think of something you could add here that wouldn't just go in the root README. Might be better to remove it.
| @@ -0,0 +1 @@ | |||
| # bot __init__ file | |||
| @@ -0,0 +1,15 @@ | |||
| # Sample Pipfile. | |||
There was a problem hiding this comment.
Nit: you can remove these sample files since they're unused.
| self.__connection = sqlite3.connect(self.name) | ||
| self.__connection.row_factory = sqlite3.Row | ||
| self.__cursor = self.__connection.cursor() |
There was a problem hiding this comment.
I have mixed feelings about the use of double underscores here, but I can buy that you may want to prevent subclasses accidentally overwriting these.
| from levels import register_all_levels | ||
| from map import Map, generate_map, image_to_discord_file | ||
| from story import StoryPage, StoryView | ||
| from utils.eval import eval_python |
There was a problem hiding this comment.
I'd argue these imports are incorrect. They only work because of the way you run your program: python bot/main.py. From the Python documentation:
If the script name refers directly to a Python file, the directory containing that file is added to the start of sys.path, and the file is executed as the __main__ module.
These imports will fail if you run with python -m, or define an entry point in pyproject.toml and install your package. The latter is generally considered best practice for packages (with one-off scripts, python script.py is fine). Your root package is bot, so absolute imports must start with that.
| data = tuple(data) | ||
| self.db.insert_many(data) | ||
| # clear new_data after saving | ||
| player.new_data.clear() |
There was a problem hiding this comment.
Does this actually do anything? Shouldn't it clear the new_plays in the history?
Overall, I'm finding this setup to be awkward. new_data doesn't semantically seem like something a Player class should it expose, but it does so to specifically support the use case of storing it in a database. Perhaps there shouldn't be this PlayerRepo middleman, and the Player should be directly save()able? Kinda like how Django ORM objects work.
| _: discord.ui.Button, | ||
| ) -> None: | ||
| """Confirm/select on the map.""" | ||
| level = Controller().get_level(self.player.get_position()) |
There was a problem hiding this comment.
I'm not sure if this is the case, but it looks like get_level may be called multiple times for the same level (both here and in get_embed_description via navigate). Perhaps an optimisation is to fetch the level once when the position updates, and then cache that. This reduces the number of DB queries you're making.
| await level().run(interaction=interaction, map=self) | ||
|
|
||
|
|
||
| def get_camera_box( |
There was a problem hiding this comment.
Most of the functions here look like they fit better in a new module for image utilities. And if you dislike the idea of things named "utility", you can call it something like "map generator" or "drawer".
| return await self._success_story(interaction=interaction) | ||
|
|
||
|
|
||
| class Level1(Level): # noqa: D101 |
There was a problem hiding this comment.
I think it'd be neater to separate the level implementations from their base class into separate modules.
|
|
||
| next_interaction = interaction | ||
| await next_interaction.response.defer() | ||
| for i, question in enumerate(self.questions): |
There was a problem hiding this comment.
This loop looks fairly complex. Are there ways to break it down and make it more readable?
No description provided.