Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Contrib/slots handler #1423

Open
wants to merge 8 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

DamnedScholar commented Sep 12, 2017

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.

DamnedScholar added some commits Sep 7, 2017

@DamnedScholar DamnedScholar Slots handler 3e5dad2
@DamnedScholar DamnedScholar Slots 41079f0
@DamnedScholar DamnedScholar Slots handler d14c64e
@DamnedScholar DamnedScholar Documentation. 3649ff4
@DamnedScholar DamnedScholar Documentation update. 296c4d1
Contributor

TehomCD commented Sep 12, 2017

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 SlottedObject or Readable in the tutorial world than, say ContribSlottedObject or TutorialReadable. I worry that with contribs with more general names, like, say, Wearable or Readable or whatever, collisions are actually fairly likely unless they're disambiguated in some way. In this case, SlottedObject is fairly unlikely unless someone was making a game that had slot machines and just decided to call a class that, but still.

Contributor

DamnedScholar commented Sep 12, 2017

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.

ContribSlottedObject would be very safe from interference, but a name like ContribWearable could still have a clash from a different contrib that implements armor in a different way. Let's say two community members want very different gear implementations, and both ideas are developed enough for contribs. One of them gets named Easy Gear and the other is called Uberarmor. Names like ContribEasyWearable and ContribUberWearable would be extremely safe against the chance of accidental conflict, without hampering readability.

@DamnedScholar DamnedScholar A little more on the readme. 14adc8e

Thanks for the contrib! I have given some code feedback inline. :)

evennia/contrib/slots_handler/slots.py
+from evennia.utils.dbserialize import _SaverDict, _SaverList, _SaverSet
+
+
+class SlotsHandler:
@Griatch

Griatch Sep 13, 2017

Owner

We are still in Python2. You should use class SlotsHandler(object): to make it a new-style class.

evennia/contrib/slots_handler/slots.py
+ def is_name_valid(self, test):
+ "Internal function to check if the name is right."
+ try:
+ exec(test + " = 1")
@Griatch

Griatch Sep 13, 2017

Owner

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.

evennia/contrib/slots_handler/slots.py
+ raise ValueError("You have to limit slot category names to "
+ "characters that are valid in variable names.")
+
+ def defrag_nums(self, cats):
@Griatch

Griatch Sep 13, 2017

Owner

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).

evennia/contrib/slots_handler/slots.py
+ 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
@Griatch

Griatch Sep 13, 2017

Owner

See the style guide for how to write the type of arguments in docstrings.

evennia/contrib/slots_handler/slots.py
+ 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.
@Griatch

Griatch Sep 13, 2017

Owner

This misses a Returns docstring section.

evennia/contrib/slots_handler/slots.py
+ """
+
+ if not isinstance(slots, (dict, _SaverDict)):
+ return StatMsg(False, "You have to declare slots in the form "
@Griatch

Griatch Sep 13, 2017

Owner

What is StatMsg and where is it defined/imported?

@DamnedScholar

DamnedScholar Sep 14, 2017

Contributor

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.
@Griatch

Griatch Sep 13, 2017

Owner

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.

@DamnedScholar

DamnedScholar Sep 14, 2017

Contributor

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.

@@ -0,0 +1,110 @@
+# Slots Handler
@Griatch

Griatch Sep 13, 2017

Owner

Great with a readme! Just to stay consistent with the rest of Evennia, rename this file to README.md (capital letters).

@Griatch

Griatch Sep 13, 2017

Owner

Also, look at the headers/readme's of all other contribs. They follow a special format where the author's name (you) is included.

evennia/contrib/slots_handler/readme.md
@@ -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.
@Griatch

Griatch Sep 13, 2017

Owner

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)
@Griatch

Griatch Sep 13, 2017

Owner

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?

@Griatch

Griatch Sep 13, 2017

Owner

I also think most of these could probably do with a brief example (not strictly necessary but helpful)

@DamnedScholar

DamnedScholar Sep 15, 2017

Contributor

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.

Owner

Griatch commented Sep 17, 2017

@DamnedScholar Please let me know when you feel the issues have been fixed and this is ready to review again. :)

DamnedScholar added some commits Sep 18, 2017

@DamnedScholar DamnedScholar Clarifying and cleaning things up c6876f3
@DamnedScholar DamnedScholar Finishing up the readme and fixing bugs that came up. 513ff7e
Contributor

DamnedScholar commented Sep 20, 2017

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment