-
Notifications
You must be signed in to change notification settings - Fork 13
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
move 'wear' action to 'garments' module #82
Conversation
player.thing, | ||
u" wrestles with basic personal problems."]))) | ||
|
||
evt = events.Success( |
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 understand that this is "just" moving the action to a different module, but while you are at it - Is there a specific reason why the name evt
is used instead of something slightly more descriptive?
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.
woohoo a code review!
I could avoid naming the variable entirely; after all, it's just created, then broadcast. No name is better than a bad name :). What do you think?
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.
👍 to "no name is better than a bad name" in this case, since events.Success(...)
makes sense to me.
LGTM. |
Thanks for re-reviewing … near instantaneously :). Will merge when CI is happy. |
In support of #55 and #54, start to split out an action from the too-long
action
module.