Skip to content
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

Closed

Conversation

DamnedScholar
Copy link
Contributor

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.

@TehomCD
Copy link
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.

@DamnedScholar
Copy link
Contributor Author

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.

Copy link
Member

@Griatch Griatch left a 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:
Copy link
Member

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
Copy link
Member

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

Copy link
Member

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.
Copy link
Member

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)
Copy link
Member

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?

Copy link
Member

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)

Copy link
Contributor Author

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")
Copy link
Member

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.

Copy link
Contributor Author

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):
Copy link
Member

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.
Copy link
Member

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
Copy link
Member

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 "
Copy link
Member

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?

Copy link
Contributor Author

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.
Copy link
Member

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.

Copy link
Contributor Author

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.

@Griatch
Copy link
Member

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
Copy link
Contributor Author

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.

@Griatch Griatch closed this Sep 23, 2017
@Griatch
Copy link
Member

Griatch commented Sep 23, 2017

Ah, didn't intend to close this. Please re-open against the develop branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants