13
\$\begingroup\$

MetaBrainz has an IRC channel called #metabrainz on which there's a bot called BrainzBot running. It provides utilities such as linking to JIRA issues or Github PRs automatically when they're mentioned. Recently, I came up with the idea of a "voting" plugin, which could keep track of community interest in a particular proposal before a formal JIRA issue is created. After a whole lot of discussion with the community, a syntax was decided, as follows:

There are two main vote types: boolean and non-boolean.

Boolean voting refers to votes where there are only two options
(along with abstaining, which I'll explain a bit later.)

<user1> !startvote <optional_name>
<BrainzBot> Voting has started.
<user1> +1
<user2> -1
<user3> -1
<user4> \1 # this is an implicit way of abstaining
<user5> !abstain
<user3> +1
<any_user> !countvotes
<BrainzBot> [+2: user1, user3] [-1: user2] [\2: user4, user5]
<any_user> !endvote
<BrainzBot> Voting has ended.

Non-boolean voting is similar, but with arbitrary options:

<user1> !startvote <optional_name> [option1, option2, option3]
<BrainzBot> Voting has started.
<user1> +option1
<user2> -option3
<user1> -option3
<user3> +option3
<user4> \1
<any_user> !coutnvotes
<BrainzBot> [option1(+1, -0): user1] [option2(+0, -0): ] [option3(+1, -2): user3; -user1, -user2] [\1: user4]
<any_user> !endvote
<BrainzBot> Voting has ended.

As you can see, users can vote for more than one option here. But note 
that just like in boolean voting they cannot vote both for and
against, the later vote overrides the earlier one.

Abstaining refers to voters who want to indicate that they are
interested in the proposal, but are explicitly neutral on it.
It's *always* an option, both in boolean and non-boolean voting.

Additional commands include !cancelvotes, which is different from
abstaining since it removes all indication that you even voted; and
the explicit !vote command, which is the same as voting implicitly
in the way that the users did in the examples above, with the key
difference being that if you say !vote option, you're
assumed to be for that option, which is not the case in the 
implicit vote.

Another thing to note is that we want to be able to match
the longest option, in case the string given the vote doesn't
directly match an option. For example:

<user1> !startvote [foo, foo bar, foo bar baz]
<BrainzBot> Voting has started.
<user1> +foo something something # Vote goes to "foo"
<user2> +foo bar something # Vote goes to "foo bar"
<user3> +foo bar bazsomething # Vote goes to "foo bar", since the "baz" doesn't have whitespace after it
<user4> +foo bar baz something # Vote goes to "foo bar baz"
<any_user> !endvote
<BrainzBot> Voting has ended.

To implement this, I have the following code. A little context before that:

The config.BaseConfig class provides configuration options that can be set by users at any time. They can be accessed using self.config["name"] in the plugin class.

The plugin class inherits from BasePlugin, which (among other non-relevant things) provides a wrapper for the Redis API (with store and retrieve methods) that plugins must use to store any values.

Finally, in order to link a function with a command, you need to decorate the function with one of a number of decorators ("command prefix" refers to "!", which can be changed, but that's handled by the plugin API):

  • listens_to_mentions(regex): A method that should be called only when the bot's nick prefixes the message and that message matches the regex pattern. For example, BrainzBot: What time is it in Napier, New Zealand?. The nick will be stripped prior to regex matching.
  • listens_to_all(regex): A method that should be called on any line that matches the regex pattern.
  • listens_to_command(cmd): A method that should be called on any line that starts with the command prefix, followed by cmd. All further arguments are passed through in a list. For example, !list ops.
  • listens_to_regex_command(cmd, regex): listens_to_command, with a regex check on all arguments.

Each function is passed in a line object which contains the user sent the line in user, and the text of the line in text. Any named regex groups are also passed in. The return value of the function is posted to the IRC channel. For example:

@listens_to_regex_command("example", r"(?P<key>.+)=(?P<value>.+)")
def example(self, line, key, value):
    return "{} passed in key {} and value {}".format(line.user, key, value)

Thank you so, so much for sticking with this if you've come this far! Finally, here's the full code:

# -*- coding: utf-8 -*-

import json
import re
from functools import wraps
from collections import namedtuple, OrderedDict

from .. import config
from ..base import BasePlugin
from ..decorators import (listens_to_all, listens_to_command,
                          listens_to_regex_command)

DEFAULT_VOTE_OPTION = "__vote__"
ABSTAIN_OPTION = "__abstain__"

ERROR_MESSAGES = {
    "no_vote_started": u"No vote has been started. Use the “startvote” command to do so.",
    "voting_already_running": u"{author}: There’s already a vote going on. Use the “endvote” command to end it before starting a new one.",
    "invalid_option": u"“{option}” is not a valid option.",
    "invalid_abstain": u"The only valid way to abstain is using \\{shortform}."
}
INFO_MESSAGES = {
    "voting_started": u"Voting has started.",
    "voting_started_name": u"Voting has started for proposal “{name}”.",
    "voting_ended": u"Voting has ended.",
    "voting_ended_name": u"Voting has ended for proposal “{name}”."
}

def get_default_vote_template(for_, against, username_sep):
    return (u"[+{number_for}: {for_users}] [-{number_against}: {against_users}]"
            .format(
                number_for=len(for_),
                number_against=len(against),
                for_users=username_sep.join(for_),
                against_users=username_sep.join(against)
            ))

def get_abstain_template(abstainers, username_sep):
    return (u"[\\{num}: {users}]"
            .format(
                num=len(abstainers),
                users=username_sep.join(abstainers)
            ))

def get_section_template(name, for_, against, username_sep):
    template = (u"[{}(+{}, -{}): "
                .format(name, len(for_), len(against)))

    if for_:
        template += username_sep.join(for_)
    if for_ and against:
        template += u"; "
    if against:
        template += username_sep.join(
            u"-" + username for username in against)
    template += u"]"
    return template


COUNTVOTE_TEMPLATES = {
    "default_vote_template": get_default_vote_template,
    "abstain_template": get_abstain_template,
    "section_template": get_section_template
}

FOR_NAME = "for"
AGAINST_NAME = "against"
ABSTAIN_NAME = "abstain"
OPPOSING_VOTE = {
    FOR_NAME: AGAINST_NAME,
    AGAINST_NAME: FOR_NAME
}
# NOTE: This ordering is not meant to represent any kind of racial distinctions
# whatsoever. It's the order and words used by the Unicode Consortium in their
# technical reports. https://www.unicode.org/reports/tr51/#Diversity
VOTE_SHORTCUT_TO_NAME = OrderedDict([
    (u"+", FOR_NAME),
    (u"👍🏻", FOR_NAME),  # Thumbs up (light skin tone)
    (u"👍🏼", FOR_NAME),  # Thumbs up (medium-light skin tone)
    (u"👍🏽", FOR_NAME),  # Thumbs up (medium skin tone)
    (u"👍🏾", FOR_NAME),  # Thumbs up (medium-dark skin tone)
    (u"👍🏿", FOR_NAME),  # Thumbs up (dark skin tone)
    (u"👍", FOR_NAME),  # Regular thumbs up
    (u"😍", FOR_NAME),  # Smiling face with heart eyes
    (u"😻", FOR_NAME),  # Smiling cat face with heart eyes
    (u"-", AGAINST_NAME),
    (u"–", AGAINST_NAME),  # En dash
    (u"—", AGAINST_NAME),  # Em dash
    (u"―", AGAINST_NAME),  # Horizontal bar
    (u"👎🏻", AGAINST_NAME),  # Thumbs down (light skin tone)
    (u"👎🏼", AGAINST_NAME),  # Thumbs down (medium-light skin tone)
    (u"👎🏽", AGAINST_NAME),  # Thumbs down (medium skin tone)
    (u"👎🏾", AGAINST_NAME),  # Thumbs down (medium-dark skin tone)
    (u"👎🏿", AGAINST_NAME),  # Thumbs down (dark skin tone)
    (u"👎", AGAINST_NAME),  # Regular thumbs down
    (u"", FOR_NAME)
])

Vote = namedtuple('Vote', ['voter', 'section', 'vote'])

class Config(config.BaseConfig):

    options_separator = config.Field(help_text="Separator to use when starting a vote with custom options", default=",")
    username_separator = config.Field(help_text="Separator between usernames when printing out votes", default=",")
    boolean_shortform = config.Field(help_text=u"The short option that voters can use in boolean votes", default="1")

class Plugin(BasePlugin):

    config_class = Config

    @listens_to_regex_command("startvote",
                              ur"(?P<name>[^\[\]]+?)?\s*(\[(?P<options>.*)\])?\s*$")
    def startvote(self, line, name, options):
        options_sep = self.config["options_separator"]
        if self.retrieve("votes"):
            return (ERROR_MESSAGES["voting_already_running"].
                    format(author=line.user))

        default_options = [DEFAULT_VOTE_OPTION]
        options =\
            filter(bool, [option_name.strip() for option_name
                          in (options.split(options_sep) if options else [])])
        options = options or default_options
        # Abstaining should always be an option
        options.append(ABSTAIN_OPTION)
        options = list(set(options))

        # NOTE: this will create a for and against in __abstain__,
        # but for simplicity's sake we won't try and prevent this.
        # All abstainers will be added to __abstain__.for.
        initial_votes = {
            option_name: {FOR_NAME: [], AGAINST_NAME: []}
            for option_name in options
        }

        self.store("name", json.dumps(name))
        self.store("options", json.dumps(options))
        self.store("votes", json.dumps(initial_votes))
        self.store("changes_since_countvote", json.dumps(False))

        return (INFO_MESSAGES["voting_started_name"].format(name=name)
                if name else INFO_MESSAGES["voting_started"])

    def _depends_on_votestarted(throw_error=True):
        def decorator(func):
            @wraps(func)
            def wrap(*args, **kwargs):
                slf = args[0]
                if slf.retrieve("votes"):
                    return func(*args, **kwargs)
                elif throw_error:
                    return ERROR_MESSAGES["no_vote_started"]
                return ""
            return wrap
        return decorator

    @listens_to_command("endvote")
    @_depends_on_votestarted()
    def endvote(self, line, args):
        name = json.loads(self.retrieve("name"))
        changes_since_countvote = json.loads(
            self.retrieve("changes_since_countvote"))
        reply = (INFO_MESSAGES["voting_ended_name"].format(name=name)
                 if name else INFO_MESSAGES["voting_ended"])
        if changes_since_countvote:
            reply += "\n" + self._print_votes()

        self.delete("name")
        self.delete("options")
        self.delete("votes")
        self.delete("changes_since_countvote")

        return reply

    @listens_to_command("countvotes")
    @_depends_on_votestarted()
    def countvotes(self, line, args):
        self.store("changes_since_countvote", json.dumps(False))
        return self._print_votes()

    escaped_voting_shortcut_pattern = (
        u"|".join(re.escape(str_) for str_ in VOTE_SHORTCUT_TO_NAME.keys()))

    vote_regex_template = (
        u"^((?P<vote>(" +
        escaped_voting_shortcut_pattern +
        u"\\\\){vote_modifier})(?P<option>.*))$")

    @listens_to_regex_command("vote", (vote_regex_template
                                       .format(vote_modifier="?")))
    @_depends_on_votestarted()
    def vote(self, line, vote, option):
        try:
            changes_made = self._add_vote(self._parse_vote(line.user, (vote, option)))
            if changes_made:
                self.store("changes_since_countvote", json.dumps(True))
        except InvalidOptionError as e:
            return unicode(e)

    # This makes the vote symbol (+, -, \, etc) required, since otherwise we can't
    # tell if this is meant to be a vote or not.
    @listens_to_all(vote_regex_template.format(vote_modifier=""))
    @_depends_on_votestarted(throw_error=False)
    def implicit_vote(self, line, vote, option):
        try:
            parsed_vote = self._parse_vote(line.user, (vote, option))
        except InvalidOptionError:
            return  # Don't complain, will generate too many false positives
        changes_made = self._add_vote(parsed_vote)
        if changes_made:
            self.store("changes_since_countvote", json.dumps(True))

    @listens_to_command("abstain")
    @_depends_on_votestarted()
    def abstain(self, line, args):
        self.store("changes_since_countvote", json.dumps(True))
        self._add_vote(self._get_abstain_vote(line.user))

    @listens_to_command("cancelvotes")
    @_depends_on_votestarted()
    def cancelvotes(self, line, args):
        self.store("changes_since_countvote", json.dumps(True))
        votes = json.loads(self.retrieve("votes"))
        votes = self._remove_all_votes(line.user, votes)
        self.store("votes", json.dumps(votes))

    def _parse_vote(self, voter, (vote, option)):
        boolean_shortform = self.config["boolean_shortform"]
        if vote == "\\":
            if (option == boolean_shortform or
                    option == ABSTAIN_NAME):
                return self._get_abstain_vote(voter)
            else:
                raise InvalidOptionError(ERROR_MESSAGES["invalid_abstain"]
                                         .format(shortform=boolean_shortform))
        if option.strip() == "":
            option = boolean_shortform

        options = self._get_valid_options()
        longest_matching_option =\
            self._find_longest_matching_option(option, options)
        if not longest_matching_option:
            raise InvalidOptionError(ERROR_MESSAGES["invalid_option"]
                                     .format(option=option))

        return Vote(
            voter=voter,
            section=(longest_matching_option.strip()
                     if longest_matching_option != boolean_shortform
                     else DEFAULT_VOTE_OPTION),
            vote=VOTE_SHORTCUT_TO_NAME[vote]
        )

    def _find_longest_matching_option(self, user_option, valid_options):
        user_option = user_option.strip().expandtabs(1)

        def matches_user_option(option):
            option = option.strip()
            # In startswith, adding the space ensures that "oreo"
            # is not treated as matching the option "o".
            return (user_option == option or
                    user_option.startswith(option + " "))

        matching_options = filter(matches_user_option, valid_options)
        return (max(matching_options, key=len)
                if matching_options else '')

    def _add_vote(self, parsed_vote):
        votes = json.loads(self.retrieve("votes"))
        voter, section, vote = parsed_vote
        option = votes[section]
        option_vote = option[vote]
        opposing_option_vote = option[OPPOSING_VOTE[vote]]

        if voter in option_vote:
            return False
        elif voter in opposing_option_vote:
            opposing_option_vote.remove(voter)

        if section == ABSTAIN_OPTION:
            votes = self._remove_all_votes(voter, votes)

        try:
            votes[ABSTAIN_OPTION][FOR_NAME].remove(voter)
        except ValueError:
            pass
        option_vote.append(voter.strip())
        self.store("votes", json.dumps(votes))
        return True

    def _print_votes(self):
        votes = json.loads(self.retrieve("votes"))
        section_names = votes.keys()
        section_names = sorted(section_names)
        # Abstain should always be last in the list
        section_names.append(section_names.pop(
            section_names.index(ABSTAIN_OPTION)))

        sections = [self._get_section_repr(section_name, votes[section_name])
                    for section_name in section_names]
        return " ".join(sections)

    def _get_section_repr(self, section_name, section):
        username_sep = self.config["username_separator"] + " "
        for_, against = section[FOR_NAME], section[AGAINST_NAME]

        if section_name == DEFAULT_VOTE_OPTION:
            return COUNTVOTE_TEMPLATES["default_vote_template"](
                for_=for_,
                against=against,
                username_sep=username_sep
            )
        elif section_name == ABSTAIN_OPTION:
            return COUNTVOTE_TEMPLATES["abstain_template"](
                abstainers=for_,
                username_sep=username_sep
            )
        else:
            return COUNTVOTE_TEMPLATES["section_template"](
                name=section_name,
                for_=for_,
                against=against,
                username_sep=username_sep
            )

    def _remove_all_votes(self, voter, votes):
        for section in votes.copy():
            sec = votes[section]
            try:
                sec[FOR_NAME].remove(voter)
            except ValueError:
                pass
            try:
                sec[AGAINST_NAME].remove(voter)
            except ValueError:
                pass
        return votes

    def _get_valid_options(self):
        options = json.loads(self.retrieve("options"))
        if DEFAULT_VOTE_OPTION in options:
            options[options.index(DEFAULT_VOTE_OPTION)] = (
                self.config["boolean_shortform"])
        return options

    def _get_abstain_vote(self, voter):
        return Vote(
            voter=voter,
            section=ABSTAIN_OPTION,
            vote=FOR_NAME
        )


class InvalidOptionError(Exception):
    pass

This code works correctly for every usecase we've tested it against, including edge cases like specifying no options, cancelling votes if you haven't voted at all, etc.

This is some very long code, and my main concern is readability and abstraction, so suggestions on that front are much appreciated. A secondary concern is the readability of the regexes, which is not the best right now. Performance is not really at a premium here, but I've kept things as fast as is reasonable.

Other minor things that are of concern:

  • Could _remove_all_votes be implemented more cleanly?
  • Is there a way to make the structure of the votes dictionary more clear?
  • Is there a cleaner way to handle abstaining (\1) in _parse_vote?
  • Is there a better way to make the vote shortcut required in implicit_vote but not in vote?
  • Is there a better way to handle diversity emojis?
  • Can _find_longest_matching_option be more explicit?

Thanks in advance!

\$\endgroup\$
  • \$\begingroup\$ I'm curious, why have you tagged this functional-programming? To me it doesn't look to employ any FP. \$\endgroup\$ – Peilonrayz Jan 23 '18 at 13:30
  • \$\begingroup\$ @Peilonrayz My goal for the style is FP, and I've used some elements that some people think are un-Pythonic, like nested functions and filter. Suggestions to push it closer to an optimal functional style are appreciated. \$\endgroup\$ – naiveai Jan 23 '18 at 13:35
8
+100
\$\begingroup\$

Grumble, grumble. Browser tab crashed and SO didn't save my draft apparently. I'll try to recreate everything I remember writing:

First off, your question is fantastically written. Often one of the most important skills in writing good code is being able to communicate your intent and any problems you face clearly. You've done an excellent job of this.

I'll start with the good:

  • Decent adherence to PEP 8
  • Good use of spacing - code is coherently grouped into "paragraphs"
  • Some comments to explain unclear intentions
  • Consistent naming patterns
  • Good use of builtins and collections (namedtuple and functools.wrap to name a few)
  • Overall pretty clean code, shows good command of python

Small points of improvement:

  • Why Python 2.7? It's days are numbered. Switching to 3 will also make some of your emoji issues a bit easier to handle
  • For such a complex system, one of the habits I've gotten into is using mypy and the new typing system. In 2.7 the syntax is cumbersome, so this really only works for 3, but I've caught so many silly errors just by adding some typing.
  • Some lines aren't wrapped to 79 cols (PEP 8).
  • Use """Docstrings.""" to document the behavior of all functions, even internal ones. Later developers (or your future self) will thank you!
  • You seem to use explicit parens when line wrapping. I tend to prefer \ as a line continuation. PEP 8 is a little vague here, but its intent seems to me that you should prefer parens for implicit line continuation only in contexts where the parens would normally be present (ex. a function call). I wouldn't insert parens around a statement to wrap it, because that creates more for your eyes to have to pair/separate out.
  • While I like your use of constants (*_MESSAGES), it has lead to kwargs to format() being passed that don't exist in the format strings (see line 116, voting_already_running and author). This is likely because of the distance between the definition and call site. Analysis tools (like pylint) might catch this. I think if you read below ("the meat"), there's a better way to address this issue.
  • What's going on with DEFAULT_VOTE_OPTION and ABSTAIN_OPTION? I see its used for serialization, but the names are bizarre, and I think there's an easier and clearer way (again see "the meat")
  • Config seems really unnecessary here. When will it ever be that you'll want to separate the usernames with something that isn't a comma? Seems exceedingly rare. When would you want to change the boolean shortform? That could only lead to confusion. Changing the custom option separator means that your plugin won't have a consistent command interface. A user would need to be aware of the configuration to know how to invoke !startvote. Configuration is good, but these options seem unnecessary (and contribute to code complexity).
  • Line 120 (options = filter(bool, [... if options else []])) is awkward, as is the construction of the options list
  • There is a preference for singly quoted strings. Double quoted strings should be reserved for format strings.
  • As you identify the regexes are a bit unwieldy
  • VOTE_SHORTCUT_TO_NAME feels awkward. It definitely doesn't need to be an OrderedDict. But it combined with all of this FOR_NAME/AGAINST_NAME stuff feels really unpythonic. I'd recommend just two tuples, SUPPORT_INDICATORS = ('+', '(unicode thumbs up)', ...) and OPPOSITION_INDICATORS = ('-', '(unicode thumbs down)', ...). Then you can do message.startswith(SUPPORT_INDICATORS).

Your syntax: As an outsider to your syntax process (and perhaps a useful perspective of a "potential user" that wasn't part of that process):

  • Your abstain syntax is confusing to me. \1 maybe makes some sense in the binary case (although I don't see how if + is support and - is reject that \ is abstain). I think it particularly breaks down, though, in the named option case where 1 no longer has any relation to any of the options.
  • Naming votes seems strange since only one can be ongoing at a time (and the name only appears to be used sometimes when printing results)
  • Having a distinction between binary and named option voting feels like more work than the benefit. I'd advocate for having !startvote with no named options automatically created one named 1. That automatically allows for +1/-1 through the name mechanism that is used for named options. When printing results you could potentially special case to get your desired output, but I'd say even that is more work for little benefit. You'd have to weigh that tradeoff on your own.
  • Can the vote be anywhere in the message? I see in your code you have a comment about suppressing error output. As a user I'd be unsure about whether my vote was counted. I'd require that votes start at the first character of a message and while voting is active display errors for any message that starts with + or - no followed by a valid option name. This way users don't have to run !countvotes after their vote to make sure it counted (and they got the syntax right), which of course would flood the chat with lots of noise.
  • I'd argue that allowing for spaces in option names is a bad idea. That means if you have an option named upgrade servers, you can no longer read a message up to the first space to determine if it is a vote. You say you want to support partially typed names, so to do that efficiently for potentially an unlimited number of space separated words, you really should employ a DAWG because the performance degrades quadratically.
  • Your explicit !vote is a bit confusing to me. Am I voting for or against the option? Seems like having two options would be easier to understand as a user: !support and !oppose. This has parity with !abstain (which I think should be your only way to abstain, with no confusing implicit option). And it also reads like a sentence when you use it !support server_upgrades.
  • Your !startvote command should instruct users on how to vote if they haven't before
  • If you take the above advice, then the space separated arguments become the optional option names for the vote (and you don't have to use any regexes or other parsing code)
  • I'd also suggest not using the suffix vote as it conflates with the !vote action. A vote is what you cast. A poll has votes cast into it. I'd call the commands !startpoll, !endpoll, and !tallypoll. Feel free to change that back, but as a potential user I'd find that nomenclature much clearer.

I continue into the meat of my analysis assuming you take all of the above recommendations. As you'll see, I think this greatly simplifies the code and also makes the system easier to use and understand from a users' perspective.

The Meat

You identify yourself that the code is "very long [...] and [your] main concern is readability and abstraction." So it seems like you're on the right path. Let's motivate that a tad. One of the first observations I made when reading your code was that it looked really good until I reached the Plugin class. It contains about 98% of your logic, and while the separate commands are somewhat separated, the concerns address by each of these commands are interwoven and duplicated in many places. Just skimming over Plugin, these concerns are easily identified, mainly because you've done a good job of phrasing your code with whitespace. So perhaps, you are even aware of these separate concerns. For example, most of the commands are responsible for a series of things:

  • Reading configuration (which as I argue above is unnecessary)
  • Retrieving serialized state
  • Decoding/constructing new state
  • Updating state
  • Serializing and saving state
  • Returning some kind of text response to the channel

That's a lot of responsibility for a single method. What's more, a lot of these points are duplicated code between the methods (especially the storage/retrieval/serialization/deserialization of the state).

What's bad about this? For one, copying and pasting is bound to produce errors eventually. Also, to modify how data is serialized, you need to modify code in many locations. Someone who isn't as familiar with the code as you may miss one. Finally it makes the code difficult to test. As you've seen the only tests that you can reasonably do are integration tests. While this may be sufficient for a plugin, I believe testable code is maintainable code.

What is a better way to structure this? It's all about separating concerns. Each class should optimally have a single responsibility. Here are the concerns I can identify in your context:

  1. Interpreting commands from IRC, delegating to the appropriate business logic, and returning a text response to the users in the channel
  2. Vote bookkeeping - adding/changing votes, tallying votes
  3. Serializing/saving/reading/deserializing the bookkeeping information from redis

Notice how each of these concerns cordons off a specific part of your application. And, critically, each only interfaces with one component. (1) doesn't interact with redis or do any bookkeeping, it delegates that to (2) and (3). (2) doesn't have to concern itself with redis things. (3) doesn't have to concern itself with what user command caused it to be invoked.

Now unfortunately, because of the way the plugin API works, it's not possible to completely separate these, but we can get pretty close, and in the process produce code that is easier to maintain and understand.

Let's first look at the Plugin itself to inform what kinds of APIs we need from the other concerns. It needs to handle !startvote, !abstain, !countvotes, !endvote, and any message containing a +/- (and emojis) vote. That would probably look something like (inline references are of the form # (xx)):

import re


SUPPORT_INDICATORS = ('+', '👍', '👍🏻', '👍🏼', '👍🏽', '👍🏾', '👍🏿')
OPPOSITION_INDICATORS = ('-', '👎', '👎🏻', '👎🏼', '👎🏽', '👎🏾', '👎🏿')


class Plugin(BasePlugin):
    @listens_to_command('startpoll')  # (1)
    def handle_start_command(self, line, option_names):  # (2)
        """Handles the !startpoll command.

        When issued with no arguments, starts a binary poll. Optional
        arguments are the option names in the poll. For example:

          <user1> !startpoll
          <BrainzBot> A new poll has been started. You can vote with !support,
                      !oppose, !abstain, or by starting your message with a +1
                      or -1. See a tally of votes with !tallypoll and end the
                      poll with !endpoll.

          <user1> !startpoll delicious_fruits vegetables
          <BrainzBot> A new poll has been started between delicious_fruits and
                      vegetables. You can vote with !support [option],
                      !oppose [option], !abstain, or by starting your message
                      with a +[option] or -[option]. See a tally of votes with
                      !tallypoll and end the poll with !endpoll.

        The command will report an error if a vote is ongoing.
        """
        if self.current_poll:
            return u'A poll is already ongoing. Issue !endpoll to end it.'

        if len(option_names) == 1:
            return u'Add 2 or more options to a poll or issue !startpoll ' \
                    'for a binary poll'

        self.current_poll = self._create_poll(option_names)

        if len(option_names) >= 2:
            return u'A new poll has started. You can vote with !support, ' \
                   u'!oppose, !abstain, or by starting your message with a ' \
                   u'+1 or -1. See a tally of votes with !tallypoll and ' \
                   u'end the poll with !endpoll'
        else:
            response = u"A new poll has started between {}. You can vote " \
                       u"with !support [option], !oppose [option], " \
                       u"!abstain, or by starting your message with a " \
                       u"+[option] or -[option]. See a tally of votes " \
                       u"with !tallypoll and end the poll with !endpoll"

            return response.format(english_list(option_names))  # (3)

    def requires_ongoing_poll(func):  # (4)
        @wraps(func)
        def wrapped(self, *args, **kwargs):
            if self.current_poll is None:
                return u'No active poll. Start one with !startpoll'

            return func(self, *args, **kwargs)
        return wrapped

    @listens_to_command('support')
    @requires_ongoing_poll
    def handle_support_command(self, line, option_names):
        self.current_poll.support(line.user, option_names)

    @listens_to_command('oppose')
    @requires_ongoing_poll
    def handle_oppose_command(self, line, option_names):
        self.current_poll.oppose(line.user, option_names)

    INDICATORS = r'|'.join(map(re.escape,
                               SUPPORT_INDICATORS + OPPOSITION_INDICATORS))

    @listens_to_all(r'^(?<indicator>(' + INDICATORS + r'))(?<option>\W+)')
    def handle_message_with_possible_vote(self, line, indicator, option):
        if self.current_poll is None:
            # Must not have been a vote
            return

        try:
            if indicator in SUPPORT_INDICATORS:
                self.current_poll.support(line.user, option)
            else:
                self.current_poll.oppose(line.user, option)
        except OptionError as e:
            return e.message

    @listens_to_command('abstain')
    @requires_ongoing_poll
    def handle_abstain_command(self, line, args):
        if len(args) > 0:
            return u'Abstain expects no arguments. Issue just !abstain'

        self.current_poll.oppose(line.user)

    @listens_to_command('tallypoll')
    @requires_ongoing_poll
    def handle_tally_command(self, line, args):
        if len(args) > 0:
            return u'Tally expects no arguments. Issue just !tallypoll'

        # NOTE: tally() returns a Tally, see below
        return str(self.current_poll.tally())

    @listens_to_command('endpoll')
    @requires_ongoing_poll
    def handle_endvote(self, line, args):
        if len(args) > 0:
            return u'Tally expects no arguments. Issue just !endpoll'

        tally = ''
        if self.current_poll.changed_since_last_tally:
            # NOTE: tally() returns a Tally, see below
            tally = u"{} ".format(self.current_poll.tally())

        self.current_poll = None
        return u"{}Poll ended.".format(tally)


class Tally(namedtuple('Tally', ('options', 'abstains'))):
    __slots__ = ()

    def __str__(self):
        # NOTE: Python 3.6 f-strings would make this much cleaner
        options = u' '.join(map(str, self.options))

        abstaining = u"[abstain ({num}): {names}]" \
            .format(num=len(self.abstains), names=', '.join(self.abstains))

        return u'{} {}'.format(options, abstaining)


class Option(namedtuple('Option', ('name', 'supporters', 'opposers'))):
    __slots__ = ()

    def __str__(self):
        # NOTE: Python 3.6 f-strings would make this much cleaner
        names = u'; '.join(u', '.join(name_list)
                           for name_list in (self.supporters, self.opposers))

        return u"[{name}(+{num_supporters}, -{num_opposers}): {names}]" \
            .format(self.name, num_supporters=len(self.supporters),
                    num_opposers=len(self.opposers), names=names)

Inline notes:

  1. I think you shouldn't do regex matching on the arguments. I'm assuming the plugin API doesn't give an error to the user if the arguments don't match the regex. I think you want to be more informative to users when the issue an invalid command. Also with my suggestions above, all commands now have space separated arguments so you can just use the last param from @listens_to_command.
  2. I've only included one doc comment as an example to make the example shorter. The rest are your job!
  3. english_list is just a helper that turns an array into a comma separated list with "and" (ex. it handles two elements just being "x and y", but also 3 being "x, y, and z")
  4. I'm still undecided on whether this use of a decorator is worth it. On one had, it removes a lot of duplicate code (used in all but one command). But on the other, it's only two lines of trivial code per method, so maybe the indirection isn't worth it.

Big takeaways:

Notice how each of the commands is now significantly simpler. They only attempt to handle parsing commands and formatting output. Their behavior is immediately apparent from a quick glance, as are their edge cases. It should be easy to modify a command's output or behavior or maybe add another command without having to understand the specifics of serialization or the poll's business logic. This deferral to the other components is also done in the most pythonic way possible. We call out to a current_poll, which if one isn't ongoing is None. A current poll can be cleared by setting to None. Also note how our use of a namedtuple as the return of tally() formally codifies a contract between the current_poll and the Plugin about how to exchange information. Additionally, we have completely removed the idea of the "changed since tally" flag. That is now entirely handled in the poll, so it is impossible to make a mistake where we don't set this flag inside the Plugin.

Another thing that you mentioned was your were unhappy with regexes. I removed most of them, because I believe the commands should error when given the wrong arguments. The only regex is the one for checking if a + or - precedes a message (indicating it may be a vote) for handle_message_with_possible_vote. We have significantly simplified the regex and that method since all polling logic has been extracted.

Now let's look at creating this current_poll object that we've been using. For now we'll skip serialization and _create_poll and focus on implementing the Poll class.

import operator


BINARY_OPTION_NAME = '1'


class Poll(object):
    @classmethod
    def create(cls, option_names):
        # Support a binary poll with no named options
        if len(option_names) == 0:
            option_names = [BINARY_OPTION_NAME]

        options = {option: (set(), set()) for option in option_names}
        return Poll(options, set(), changed_since_last_tally=True)

    @classmethod
    def from_json(cls, serialized):
        data = json.loads(serialized)

        options = \
            {name: (set(supporters), set(opposers))
             for name, (supporters, opposers) in data['options'].iteritems()}
        abstains = set(data['abstains'])
        changed_since_last_tally = data['changed_since_last_tally']

        return Poll(options, abstains, changed_since_last_tally)

    def __init__(self, options, abstains, changed_since_last_tally):
        self._options = options
        self._abstains = abstains
        self._changed_since_last_tally = changed_since_last_tally

    def support(self, user, partial_option_name=None):
        supporters, opposers = self._get_option(partial_option_name)

        if user in supporters:
            # No changes necessary
            return

        self._clear_vote(user)
        supporters.add(user)
        opposers.remove(user)
        self._changed_since_last_tally = True

    def oppose(self, user, partial_option_name=None):
        supporters, opposers = self._get_option(partial_option_name)

        if user in opposers:
            # No changes necessary
            return

        self._clear_vote(user)
        supporters.remove(user)
        opposers.add(user)
        self._changed_since_last_tally = True

    def abstain(self, user):
        if user in self._abstains:
            # No changes necessary
            return

        # Clear any other votes the user has cast
        for supporters, opposers in self._options.itervalues():
            supporters.remove(user)
            opposers.remove(user)

        self._abstains.add(user)
        self._changed_since_last_tally = True

    def _get_option(self, partial_option_name):
        if partial_option_name is None:
            if len(self._options) != 1:
                raise OptionError(partial_option_name, self._options.keys())

            return self._options[BINARY_OPTION_NAME]

        # Consider options in sorted order, finding the first that has
        # partial_option_name as a prefix to support partially typed option
        # names
        options = sorted(self._options.iteritems(),
                         key=operator.itemgetter(0))
        for option_name, (supporters, opposers) in options:
            if option_name.startswith(partial_option_name):
                return supporters, opposers

        raise OptionError(partial_option_name, self._options.keys())

    def tally(self):
        self._changed_since_last_tally = False

        options = \
            [Option(name, list(supporters), list(opposers))
             for name, (supporters, opposers) in self._options.iteritems()]
        return Tally(options, list(self.abstains))

    @property
    def changed_since_last_tally(self):
        return self._changed_since_last_tally

    def to_json(self):
        data = {'options': self._options,
                'abstains': self._abstains,
                'changed_since_last_tally': self._changed_since_last_tally}

        return json.dumps(data)


class OptionError(Exception):
    def __init__(self, bad_name, options):
        if len(options) == 1:
            msg = u'poll is binary (try +1 or -1, instead of a named option)'
        else:
            msg = u'expected one of the following options: {}' \
                .format(u', '.join(options))

        super(OptionError, self).__init__(msg)

        self.bad_name = bad_name
        self.options = options

You'll noticed I've omitted the docstrings, because this answer has gotten really long. You should include them. Notice how this class only concerns itself with tracking support and opposition for options and abstaining votes? No mention of any commands or serialization! You should be able to test this in isolation to make sure you voting mechanics work (and you should, because I haven't!). The Poll.create() class method may look a little strange. I'm a big believer in logicless constructors for testability. Users of Poll should never create a new one on their own. They should invoke Poll.create(). But for tests, you can create one at will with some state already setup to see how it behaves. This also acts as a nice entry point for our serialization methods that we'll add later.

Some wins for this separation are that the logic for finding the option a user meant is centralized, as is the logic for ensuring a user hasn't voted for anything else. All of the vote logic is now close to each other, so making changes to voting mechanisms should be easier (because a change's impact can only affect the surrounding code).

One thing to note is that in designing the Poll API, we've exposed an exception that we need to handle and tweaked how the partial_option_name param to the support and oppose methods work. Namely, partial_option_name can be none (in the case of a binary poll) or a string (the possibly partial name of the option). Updating our Plugin is easy:

@listens_to_command('support')
@requires_ongoing_poll
def handle_support_command(self, line, args):
    if len(args) > 1:
        return u'Support expects either no arguments or one argument.'

    try:
        partial_option_name = next(args, None)
        self.current_poll.support(line.user, partial_option_name)
    except OptionError as e:
        return e.message

@listens_to_command('oppose')
@requires_ongoing_poll
def handle_oppose_command(self, line, args):
    if len(args) > 1:
        return u'Oppose expects either no arguments or one argument.'

    try:
        partial_option_name = next(args, None)
        self.current_poll.oppose(line.user, partial_option_name)
    except OptionError as e:
        return e.message

Now at this point to get something working all we need to do is define _create_poll:

def _create_poll(self, option_names):
    return Poll.create(option_names)

And in fact, we can just remove the _create_poll method at this point. It only had one use and it was mainly a placeholder while we worked out the specifics of the Poll API. We'll just replace it's callsite with Poll.create:

    self.current_poll = Poll.create(option_names)

At this point, everything should be working. The plugin and all its commands should just work. We have a well designed set of components that handle single concerns and are not tightly coupled to eachother's implementation. We haven't implemented any serialization though. And honestly, this would be where I would stop. Unless it is really common for the bot to crash or polls take hours instead of minutes, I'd say the effort to serialize isn't worth the complexity it adds. But if you need it, read on:

Serialization is always a tricky subject, because it often breaks some rules. Ideally we'd like our Poll to not be concerned with serialization, but for this simple use case it is easier and cleaner to add serializing and deserializing to Poll:

@classmethod
def from_json(cls, serialized):
    data = json.loads(serialized)

    options = \
        {name: (set(supporters), set(opposers))
         for name, (supporters, opposers) in data['options'].iteritems()}
    abstains = set(data['abstains'])
    changed_since_last_tally = data['changed_since_last_tally']

    return Poll(options, abstains, changed_since_last_tally)


def to_json(self):
    data = {'options': self._options,
            'abstains': self._abstains,
            'changed_since_last_tally': self._changed_since_last_tally}

    return json.dumps(data)

Now that a Poll knows how to serialize itself. We'd really like to extract the serialization behavior. We don't want it in Poll, but we also don't want it in Plugin (since the responsibility of that class is command parsing, delegating, and printing output). We note that after a each (public) method call to Poll we should serialize it to Redis since a change is possible (we assume here that saving a poll that hasn't been modified is of negligible impact). So, we want to decorate the methods of Poll so that after they are called, the poll is saved to Redis. For this we look to the decorator pattern. Namely, we want to make a class which will defer all method calls and property accesses to another object, but after all method calls, we want to add some special behavior. There's a bit of Python magic here, but I'd say the tradeoff is worth it since it gives you such expressive and storage-independent syntax for querying and updating the poll.

from inspect import ismethod


class PersistentDecorator(object):
    def __init__(self, decorated_object, save):
        """A decorator that calls save after every method call on decorated_object."""
        self._decorated_object = decorated_object
        self._save = save

    def __getattribute__(self, attr):
        if attr.startswith('_'):
            return super(PersistentDecorator, self).__getattribute__(attr)

        value = getattr(self._decorated_object, attr)

        if not ismethod(value):
            return value

        @wraps(value)
        def wrap(*args, **kwargs):
            return_value = value(*args, **kwargs)
            self._save(self._decorated_object.to_json())
            return return_value
        return wrap

There's a bit to unpack here. But the idea is that you'd use it like this: PersistentDecorator(Poll.create(options), partial(self.store, 'data')). PersistentDecorator behaves exactly like decorated_object. It has all of the same properties and methods. However, whenever you call one of the methods, the save method is invoked afterwards. For example:

poll = Poll.create([])
poll.support('some_user')  # this is not saved

poll = PersistentDecorator(poll, partial(self.store, 'data'))
poll.oppose('some_user')  # the oppose logic executes, then is saved to redis

Python doesn't make this pattern as easy as it could be (see Ruby's SimpleDelegator), but the ugliness is contained to this class. If used this approach in production in a few places, and for simple cases like this is works just fine.

To use our new decorator we must wrap any polls we create. Since we want to keep the nice, expressive self.current_poll syntax, the logical place would be to make that a computed property of Plugin that handles the wrapping:

from functools import partial


STORAGE_KEY = 'data'


class Plugin:
    # ... snip ....

    @property
    def current_poll(self):
        # If _current_poll doesn't exist, a poll may be in storage
        if not hasattr(self, '_current_poll'):
            self._current_poll = self.retrieve(STORAGE_KEY)

            # If there was a poll in storage, deserialize it
            if self._current_poll is not None:
                self._current_poll = \
                    self._wrap_poll(Poll.from_json(self._current_poll))

        return self._current_poll

    @current_poll.setter
    def current_poll(self, poll):
        self._current_poll = self._wrap_poll(poll)
        self.store(STORAGE_KEY, poll.to_json())

    def _wrap_poll(self, poll):
        return PersistentDecorator(poll, partial(self.store, STORAGE_KEY))

Note: This assumes that self.retrieve returns None for a key that doesn't exist. Assuming retrieve just returns the GET command result from Redis, I believe this is the default behavior for most redis clients. If not, it's easy enough to modify this to handle how self.retrieve behaves.

Now without making any other changes, we have a poll that serializes as we update it. This is a really elegant concept. Should we want to swap out the persistence layer or remove it entirely, we only need to modify this property. We also note that on first access, we try to read the poll from the persistence layer. This is a little jagged of an edge, although since we extracted the decorator, I content this is still rather clean. Since the Plugin is the only object that has access to store and retrieve, we are somewhat forced to somewhat handle persistent within it. But we've successfully extracted it away to a small corner of the object.

The whole code is in this gist, in case you want to see it all together. Separation of concerns also has the benefit that we can separate this behavior into three more-manageable files.

So, I think that's about it. I was pretty aggressive and opinionated with my refactorings and I skipped a few steps, because this is answer already way too long. But that should be the gist.

tl;dr Separation of concerns/SRP is your friend; it makes code easy to maintain/read/test

\$\endgroup\$
  • \$\begingroup\$ Oops! Indeed. Nice catch. Fixed in post and gist! \$\endgroup\$ – Bailey Parker Jan 25 '18 at 9:38
  • \$\begingroup\$ I too very much hate Python 2.7, but in this case unfortunately the server we're running this on (alone with the surrounding infrastructure code) is still stuck. :( But this is an awesome answer! Thanks a lot for all the detail you go into :D \$\endgroup\$ – naiveai Jan 25 '18 at 17:16
  • \$\begingroup\$ voting_already_running does have the {author} formatting in it, though. \$\endgroup\$ – naiveai Jan 25 '18 at 17:26
  • \$\begingroup\$ No problem! Hope it was helpful. Maybe porting to python 3 is your next project then ;) Yea looking back voting_already_running did have {author}. That was from memory, but I think there was at least one format string with a bad arg. In any event, pylint will help, but only if the string is a literal at the format callsite (iirc). \$\endgroup\$ – Bailey Parker Jan 25 '18 at 22:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.