brendacs / rolley-py Goto Github PK

1. docstrings

• Instead of hash comments above a function, use standard docstrings:

- Original:

# check if role is accessible
def is_accessible_role(role):
    if role in INACCESSIBLE_ROLES:
        return False
    return True

- Docstring:

def is_accessible_role(role):
    """ Check if role is accessible """

    if role in INACCESSIBLE_ROLES:
        return False
    return True

• Since your functions names are already self-descriptive, you may just want to specify arg types / return types in the docstrings (if appropriate)

2. return statements

• Empty return statements are ok, especially if they are used to explicitly indicate that a function is terminating at that point. However, it's also common to leave out return statements when a function "naturally" ends, because None will be returned, anyway (all python functions return None unless something else is explicitly returned)

Example: these three functions behave identically, and they all return None:

def foo(s):
    print(s)
    return None

def bar(s):
    print(s)
    return

def baz(s):
    print(s)

• Whichever style you choose, it should be used consistently. In roles.py, both styles are being used. remove_role() uses a (blank) return at the bottom of the function, but remove_all_roles() does not.

The structure of the nested dictionaries in ROLES is essentially inverted/reversed. This has turned what should be O(1) operations in to O(N) operations.

For example, consider the following function from emojis.py:

def is_clearing_emoji(emoji):
    clearing_emojis = list(ROLES["clears"].values())
    if emoji in clearing_emojis:
        return True
    return False

At the key, 'clears', there is a nested dictionary. The values from the nested dictionary are retrieved as a list. The code then iterates over the list to check if emoji is in it.

(Note: the call to list() is superfluous, because the .values() method already returns a list)

What if the keys and values were reversed? If the emojis are the keys themselves, then we can check if an emoji is "in" the dictionary as an O(1) operation:

def is_clearing_emoji(emoji):
    return emoji in ROLES['clears']

I have inverted the mapping and made it available here. Committing it to the repo will require an update to virtually every function that accesses ROLES.

Making this change will not only improve performance, but it will also simplify pretty much every function that uses ROLES.

Hi,
Don't I need the .env file?
Where should I setup the bot token?
dotenv_path = join(dirname(__file__), '.env') load_dotenv(dotenv_path) TOKEN = os.environ.get('TOKEN')
Thanks

- config.py:
INACCESSIBLE_ROLES = ["the imagineers", "Admin", "mods", "Interviewers",
                      "Recruiter", "Hiring Manager", "Bot Creation", "Website Mod"]

- roles.py:
# check if role is accessible
def is_accessible_role(role):
    if role in INACCESSIBLE_ROLES:
        return False
    return True

INACCESSIBLE_ROLES is an unordered list. Checking if a value exists in an unordered list is slow (it requires a linear scan through the list, comparing each list element to the target value).

Solution: use a set ({}), not a list ([]):

- config.py:
INACCESSIBLE_ROLES = {"the imagineers", "Admin", "mods", "Interviewers",
                      "Recruiter", "Hiring Manager", "Bot Creation", "Website Mod"}

- roles.py:
def is_accessible_role(role):
    return role not in INACCESSIBLE_ROLES    # will return True/False

1. Embed() class already available in discord package

• An Embed() class already exists. You use it in commands.py. Why create a custom class?

• A custom class makes sense if the existing one is too heavyweight, but I don't believe that applies here.

2. Use a plain function

• If you switch to discord.Embed(), then you only need a simple function (plus optional wrapper):

import discord
from utils.config import EMBEDS

def _generate_embeds(data):
    """ Returns a list of discord.Embed objects generated from the input data """

    embeds = []
    for datum in data:
        title, descrip = datum
        emb = discord.Embed(title=title, description=descrip)
        embeds.append(emb)
    
    return embeds

def get_embeds():
    """ Directly returns the list of Embed objects """
    
    return _generate_embeds(EMBEDS)

If the bot loses connection to the server, it cleans the old messages and add new ones. Even removing the "run_cleanup" on start it don't accept reactions on old messages. This is an issue because sometimes disconnect servers crashes or even my server can lose connection to discord and all people that had reacted to a message will lose the reaction but keep the role. The bot should be able to keep the old messages and still be able to "manage" roles from the old reactions.
Sorry for bad english...

1. is_accessible_role()

• The function can be written more concisely:

def is_accessible_role(role):
    return role not in INACCESSIBLE_ROLES:

• A larger problem is that it is performing an exact string comparison. If someone attempts to use an inaccessible role with modified capitalization, (e.g.: "The Imagineers" instead of "the imagineers"), the function will return True.

• Simple solution: use lowercase strings in INACCESSIBLE_ROLES, and modify the above function:

def is_accessible_role(role):
    return role.lower() not in INACCESSIBLE_ROLES:

• Problems:
  • " The Imagineers " (leading/trailing spaces)
  • "The Imagineers" (embedded space)
  • "The 1magineers" (letter to number conversion)
  • Using unicode to find visually-similar character replacements

This is a complex problem, and probably not worth working on for now.

2. async def remove_role()

• This could be rewritten like so:

async def remove_role(bot, user, role):
    if is_valid_role(user, role) and user_has_role(user, role):
        for r in user.roles:
            if r.name == role:
                try:
                    await bot.remove_roles(user, r)
                except discord.Forbidden:
                    await bot.send_message(user, "Please give bot Role permissions...")
                
                break    # since role was found and removal was attempted

• Also: if a user has a role, it must be valid, no? So why bother checking if the role is valid?

• The is_valid_role() function is already iterating over user.roles to find a match. So why perform a second check of the exact same thing in remove_role() ?