FuelRats / pipsqueak

ED Fuel rats IRC bot
Other
13 stars 14 forks source link

Fix MECHA-230 #240

Closed theunkn0wn1 closed 6 years ago

theunkn0wn1 commented 6 years ago

Fixes MECHA-230 by replacing all instances of 'You need to be a registered and drilled Rat to execute this command!', 'Sorry, you need to be a registered and drilled Rat to use this command.' and 'Sorry, but you need to be a registered and drilled Rat to use this command.' with the variable api.names.msg_not_idented.

api.names.msg_not_idented is defined as string "Sorry, but you need to be a registered and drilled Rat with an identified IRC nickname to use this command."

The reason i redefined all the hardcoded strings to a variable is because it increases uniformity (we had 3 different messages stating the same thing!) and makes it more maintainable. If i went and search-replaced all the hardcoded string instances the next change would also have to be search-replace. Placing it as a single var means we just have to edit the string in one place rather than n cases

Update:

Refactored each @require_{role} decorator to have a default message, and changed all decorated instances to use the default, excluding those that specifically had no message.

theunkn0wn1 commented 6 years ago

Well, it already has a default value for the optional parameter. According to the decorators top-level docstring:

def require_rat(message=None):
    """Decorate a function to require the triggering user to be a FuelRats rat (as in, registered with the API and drilled).
    If they are not, `message` will be said if given.""

the default is no message, None and, if the message is not None it prints it if the user fails the auth check. Granted I could find no instances of @require_rat with no given param, it made sense to have the default be None. However, I do see the value in having to evaluate the string only once, and not at every instance of the decorator. I will implement the change, test it, and push to this repo in a few hours time

theunkn0wn1 commented 6 years ago

Should i also refactor the other require_{roll} decorators whilst im here? They all have essentially the same problem.

Marenthyu commented 6 years ago

Yes, please. Would be good if we're at it!


From: Orion notifications@github.com Sent: Monday, November 20, 2017 6:40:11 PM To: FuelRats/pipsqueak Cc: Marenthyu; Comment Subject: Re: [FuelRats/pipsqueak] Fix MECHA-230 (#240)

Should i also refactor the other require_{roll} decorators whilst im here? They all have essentially the same problem.

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FFuelRats%2Fpipsqueak%2Fpull%2F240%23issuecomment-345771258&data=02%7C01%7Cpetersilie93%40live.de%7C20a7527b62984443d5c308d5303dc147%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636467964142306083&sdata=wFtz6c7VpkaOLhbEtkDEJaK2m5hVmRZkKlQ%2BkRzZLwg%3D&reserved=0, or mute the threadhttps://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAIhR1_cc5LCxA_WfBemjIMt7nx0T7J97ks5s4bl7gaJpZM4Qjnbg&data=02%7C01%7Cpetersilie93%40live.de%7C20a7527b62984443d5c308d5303dc147%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636467964142306083&sdata=zZ0j7eRoY9CC4%2FYh0%2Bis3BOo%2BtP%2Bw6KQ1ZsVbw0ZF2g%3D&reserved=0.

theunkn0wn1 commented 6 years ago

Refactoring complete. I actually see room for improvement here. it seems the different require_{role} decorators are all the same except for the

if getPrivLevel(trigger) < privilage_level_int

line of each one. Honestly why is this not a single decorator with the privilege level as an argument? We could define an enum class Privilege with fields Privilege.Techrat, Privilege.Rat etc This would reduce redundancy and potential errors that may bring.

Edit: its worth noting that while i know some Python, im by no means a Pro just yet. Which is why im asking rather than simply doing it

theunkn0wn1 commented 6 years ago

and some unrelated changes found their way out of the stash and into here. Don't merge yet. Fixed. User did not understand what stash does

theunkn0wn1 commented 6 years ago

@Marenthyu Can i get this PR reviewed sometime soon? before Firebeam tries to break it again?