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
Contrib/slots handler #1423
Contrib/slots handler #1423
Conversation
One thing I noticed when running tests that might be worth discussing going forward is possibly making any typeclass in contribs have as specific a name as possible to prevent collisions in games when using the test runner: any typed object is treated as being from the same application no matter where they come from. The problem is that any name clashes will stop tests from running, which is far more likely with something like |
Fair point. And since the names only have to be highly specific in test files, we can enforce strict namespace rules without negatively impacting the experience of developers using the contribs.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contrib! I have given some code feedback inline. :)
from evennia.utils.dbserialize import _SaverDict, _SaverList, _SaverSet | ||
|
||
|
||
class SlotsHandler: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are still in Python2. You should use class SlotsHandler(object):
to make it a new-style class.
@@ -0,0 +1,110 @@ | |||
# Slots Handler |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great with a readme! Just to stay consistent with the rest of Evennia, rename this file to README.md
(capital letters).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, look at the headers/readme's of all other contribs. They follow a special format where the author's name (you) is included.
@@ -0,0 +1,110 @@ | |||
# Slots Handler | |||
It's pretty common for RPG systems to have mechanics where a character is limited to, or must choose, a specific number of items. This contrib is designed to be adaptable to handle as many permutations of this style of mechanic as possible. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know you go into this in more detail later but maybe a short example of how this could look in-game might be useful here in the introduction?
|
||
## SlottedObject.slots.attach | ||
``` | ||
attach(self, target, slots=None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading this as a new potential user I don't understand what "attach the target in all slots" means. Why would you want to regularly add the target object to all slots? Could you elaborate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also think most of these could probably do with a brief example (not strictly necessary but helpful)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading this as a new potential user I don't understand what "attach the target in all slots" means.
The full clause is "attach the target in all slots it consumes", but that's probably not the best way to word it. I'll make sure to talk about it more thoroughly in the walkthrough. The reason for the "all slots" wording is the fact that the attach()
method will only work if every one of the slots requested is available.
def is_name_valid(self, test): | ||
"Internal function to check if the name is right." | ||
try: | ||
exec(test + " = 1") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest this entire check should be removed. If this is code only intended for developers, this check is pointless - if a wrong variable name is used a traceback will catch it.
If this contrib is used by some overlying build command it's much worse - using exec()
- and completely without checking the input too - you are now opening up yourself to a huge security exploit - if the input test
(for example passed from some build command to slots.add or slots.delete
) is in any way in the hands of an untrusted user they can now execute Python code unchecked on your server - the same power as @py
. Even if you don't intend for more than internal use, the use of exec()
is not advertised anywhere and may not be caught and the input properly verified by an user of this contrib. So, please remove this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
raise ValueError("You have to limit slot category names to " | ||
"characters that are valid in variable names.") | ||
|
||
def defrag_nums(self, cats): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is not a private function, it should have a Google style docstring as per the contribution guidelines. This is true for all methods and functions in the contrib (I won't mark them each explicitly).
Args: | ||
slots: A dict of slots to add. Since you can't add empty | ||
categories, it would be pointless to pass a list to this | ||
function, and so it doesn't accept lists for input. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This misses a Returns
docstring section.
Create arrays of slots, or add additional slots to existing arrays. | ||
|
||
Args: | ||
slots: A dict of slots to add. Since you can't add empty |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the style guide for how to write the type of arguments in docstrings.
""" | ||
|
||
if not isinstance(slots, (dict, _SaverDict)): | ||
return StatMsg(False, "You have to declare slots in the form " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is StatMsg
and where is it defined/imported?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies. I've been playing around with a status message object and forgot to remove the references to it. I've changed all of those to raise Exception()
.
Args: | ||
target (object): The object to be attached. | ||
slots (list or dict, optional): If slot instructions are given, | ||
this will completely override any slots on the object. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A Returns:
section is missing (it's missing all over, so won't point those out henceforth). An Examples:
section is optional but may be useful too here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not going to include Examples:
in the docstrings because I feel like they would take up too much space and I can include a more thorough tutorial in the README
.
@DamnedScholar Please let me know when you feel the issues have been fixed and this is ready to review again. :) |
Alright. I've added a walkthrough to the readme that should make it a breeze for new people to get into working with the contrib. Everything has better docstrings and in replicating the real-world use I ran into an edge case bug that would have been pretty embarrassing. So that's good. |
Ah, didn't intend to close this. Please re-open against the |
Brief overview of PR additions
This contrib introduces SlotsHandler, a system for registering an arbitrary number of slots on a typeclassed object. It's intended to be used as a storage system by game system commands, where any sort of Python object can be recorded in each slot. The basic use case of this is to provide a limiter for entities like magic items and spells, but it is as generic as possible, so it should be useful for a broad array of systems.
Motivation for adding to Evennia
The behavior required for slot-based mechanics isn't strongly supported, and this handler serves as a wrapper that performs management operations to allow for a developer to easily create/delete slots and fill/empty them.