Wily Wolves#39
Conversation
* Use Actions that does not require credentials The GitHub Actions that we were planning on using used GitHub credentials to make comments on Pull Requests. Unfortunately, using credentials is disabled on Pull Requests made from a fork. That's why were dropping down to a regular CI flake8 check that does not create fancy comments. * Use correct line length of 119 for linting Co-authored-by: Sebastiaan Zeeff <sebastiaan.zeeff@gmail.com> Co-authored-by: Sebastiaan Zeeff <33516116+SebastiaanZ@users.noreply.github.com>
|
Thank you for contributing to Python Discord! Please check out the following documents:
|
ghost
added
the
Scaffold
Account actions (register, login, etc.)
Adds players model to the game
Adds locations and NPCs to the DB
Adds items and ownership for items
…e-jam-2020 into python-discord-master
Python discord master
Have the project run in a docker container
Update README with instructions to setup the game
|
Needs manual merge:
No flake8 linting errors |
SebastiaanZ
added
the
Manual Merge
Den4200
left a comment
Den4200
left a comment
There was a problem hiding this comment.
Thanks for the submission, Wily Wolves, and thanks for participating in our code jam!
Readme
The readme for the project was good! An adequate description of the project was given, along with simple setup instructions and how to play the game. The only thing missing is screenshots, but overall, nice job here.
Commit Quality
The commit quality for this project is okay. Most commits are descriptive enough, so that someone could figure out what each commit did. However, I did notice a few things. Some commits were described inaccurately. There were three commits titled "create engine to handle command lines". The first of those three was fine, but the second is described incorrectly, since it just removes a comment, and the third is commit is also inaccurate, since it changes a consumer. Those should've been titled correctly.
I also noticed quite a few consecutive commits that were solely for fixing linting issues. These should've been squashed down to one commit.
It would've been nice to see some commit bodies for a few of the larger commits, further explaining what it changed.
Use of Python and Code Quality
Documentation
There is almost no documentation anywhere, except on the models. There are plenty of places which could've benefited from documentation.
Code Formatting
All the code here passed our flake8 linting, very nice. I only noticed a few issues with formatting.
Here is one method that is extremely large.
Not only is this way too long, there's also several levels of nesting happening here. This should've been broken up into different methods.
There were also a couple of unused auto-generated files that should've been removed.
Testing
There were no tests found for this project. That's okay, but it would've been nice to see some.
Use of Django
The use of Django in this project was fine - I didn't find anything particularly amazing or bad. There is one thing I want to point out however, the naming of migration files. The default names, such as 0002_auto_20200805_1214.py are not descriptive at all. They should be changed to be named something descriptive, such as 0002_create_location_and_player.py.
The complete picture
Overall, the code quality was mediocre. Most of the code does not have documentation or type annotations. There were some unused auto-generated files that were not removed. There weren't any tests, the project was using the default, indescriptive migration names, and there was one extremely large method that should've been broken up into several other methods. I also noticed some odd inconsistencies, which I commented on below.
Project Stack
There was good use of docker and docker-compose to manage containers. It was quite simple to get the project up and running.
Something odd I noticed was that there was use of both pipenv and a requirements.txt. There is no need to have both, and you all should've stuck to one (preferably a dependency/environment manager such as pipenv).
Django's built-in development server was used, instead of a web server gateway interface (WSGI) such as uWSGI or gunicorn. Using one of those would make your app much more secure and scalable.
For the frontend, this project uses Django templates instead of a JS framework, which is perfectly fine. The frontend ended up looking quite cool nevertheless.
Overall, the project stack for this project was fine.
Pros
- Fine project stack
- Solid readme
- Most commits were descriptive enough
Cons
- Some commits were described inaccurately
- Unused auto-generated files not removed
- Really long method - should've been split up
- Lack of documentation and type annotations
- Default, unchanged migration names
- A few code style inconsistencies here and there commented on below
| [dev-packages] | ||
| flake8 = "*" | ||
|
|
||
| [packages] | ||
| django = "*" | ||
| psycopg2 = "*" |
There was a problem hiding this comment.
It's best to pin your dependencies at a specific version. I typically use ~= instead of ==, so it'll only update for minor versions.
| # from django.contrib import admin | ||
|
|
||
| # Register your models here. |
| # from django.db import models | ||
|
|
||
| # Create your models here. |
| # from django.test import TestCase | ||
|
|
||
| # Create your tests here. |
| from game.models import Player, Location | ||
|
|
||
|
|
||
| class Engine(): |
There was a problem hiding this comment.
There's no need to include the parenthesis for classes if you don't explicitly inherit another class.
| class Engine(): | |
| class Engine: |
| # SECURITY WARNING: keep the secret key used in production secret! | ||
| SECRET_KEY = '_95b@3cau&-)w7t2uoie$$vf6+z!-%h$=5*_l3-0p&n@0bg&c8' | ||
|
|
||
| # SECURITY WARNING: don't run with debug turned on in production! | ||
| DEBUG = True |
There was a problem hiding this comment.
These two should be environment variables, so you wouldn't have to edit the code to run it in production.
| from pyfiglet import figlet_format | ||
| from django.contrib.auth.models import User | ||
| from game.models import Player | ||
| from channels.auth import (login as auth_login, logout as auth_logout) |
There was a problem hiding this comment.
The parenthesis are unnecessary.
| self.channel_name | ||
| ) | ||
|
|
||
| # Receive message from WebSocket |
There was a problem hiding this comment.
This should be a docstring instead.
| def receive(self, text_data): | ||
| text_data_json = json.loads(text_data) | ||
| message = text_data_json['message'] | ||
|
|
||
| if message: | ||
| parsed_command = message.split(maxsplit=1) | ||
| command = parsed_command[0].lower() | ||
| try: | ||
| command_arguments = parsed_command[1:] | ||
| except IndexError: | ||
| command_arguments = [] | ||
|
|
||
| if self.scope['user'].is_authenticated: | ||
| try: | ||
| return_message = getattr(self.engine, command)(*command_arguments) | ||
|
|
||
| except AttributeError: | ||
| # engine has no attribute command | ||
| return_message = f"{command} is not a valid command! Type 'help' if you need." | ||
|
|
||
| elif command == 'login': | ||
| if len(message.split()) == 3: | ||
| username_to_login = message.split()[1] | ||
| plain_password_to_login = message.split()[2] | ||
|
|
||
| if len(User.objects.filter(username=username_to_login)) == 1: | ||
| self.user = User.objects.get(username=username_to_login) | ||
| dt = self.user.last_login | ||
|
|
||
| if django_pbkdf2_sha256.verify(plain_password_to_login, self.user.password): | ||
| async_to_sync(auth_login)(self.scope, user=self.user) | ||
|
|
||
| if dt: | ||
| return_message = ( | ||
| f"Welcome back, {self.user}! \n" | ||
| f"You last logged in at {dt.strftime('%Y-%m-%d %H:%M')} (UTC)" | ||
| ) | ||
|
|
||
| else: | ||
| return_message = ( | ||
| f"Welcome to the MUD, {self.user}! \n" | ||
| f"Since it's your first time here, we'll guide you in your first steps." | ||
| ) | ||
|
|
||
| async_to_sync(self.channel_layer.group_send)( | ||
| self.room_group_name, | ||
| { | ||
| 'type': 'global_message_login_required_not_me', | ||
| 'message': f"{self.user} is back to WilyWolves MUD!", | ||
| 'sender_channel_name': self.channel_name | ||
| } | ||
| ) | ||
|
|
||
| else: | ||
| return_message = "Wrong password! Please try 'login <username> <password> again." | ||
|
|
||
| else: | ||
| return_message = ( | ||
| f"{username_to_login!r} is not a valid username. " | ||
| "If you are new here, please type 'new'" | ||
| ) | ||
|
|
||
| else: | ||
| return_message = "To log in, please type 'login <username> <password>'." | ||
|
|
||
| elif command == 'new': | ||
| if len(message.split()) == 3: | ||
| username_to_create = message.split()[1] | ||
| password_to_create = message.split()[2] | ||
| hashed_password = make_password(password_to_create) | ||
|
|
||
| if len(User.objects.filter(username=username_to_create)) == 0: | ||
| new_user = User( | ||
| username=username_to_create, | ||
| password=hashed_password, | ||
| is_superuser=False, | ||
| is_staff=False | ||
| ) | ||
| new_user.save() | ||
| new_player = Player(user=new_user) | ||
| new_player.save() | ||
| return_message = ( | ||
| f"User {username_to_create!r} successfully created! " | ||
| "Please type 'login' to start playing." | ||
| ) | ||
|
|
||
| else: | ||
| return_message = f"Someone is already using {username_to_create}" | ||
|
|
||
| else: | ||
| return_message = "To create a new user, please type 'new <username> <password>'." | ||
| else: | ||
| return_message = "You need to log in first. Please type 'login' or 'new'" | ||
|
|
||
| if return_message is not None: | ||
| self.send(text_data=json.dumps({ | ||
| 'message': return_message | ||
| })) |
There was a problem hiding this comment.
This method is way too big for my liking. I feel that the logic here should've been split into multiple methods, instead of squishing it all into one.
|
|
||
|
|
||
| class ItemBlueprint(models.Model): | ||
| '''The "blueprint" for items in the game.''' |
There was a problem hiding this comment.
The quotes should be consistent. These single quotes should be double quotes, similar to the other docstrings.
5 participants
No description provided.