SpockBotMC / SpockBot-Extra

Extra plugins and examples for SpockBot
2 stars 2 forks source link

Added tpa and tpaccept #7

Closed JerryHigarr closed 9 years ago

JerryHigarr commented 9 years ago

I have been coding a large number of commands for my vanilla server using this project, so I wanted to show my appreciation by offering a couple of them :)

JerryHigarr commented 9 years ago

Hopefully that's a tad more acceptable :)

txomon commented 9 years ago

Apart from the formatting, it looks good to me, but I haven't tested it.

JerryHigarr commented 9 years ago

I just tested it, and I fixed some things. A big thanks to @Gjum who has been very understanding.

JerryHigarr commented 9 years ago

added a flake8 fix thanks to @txomon :)

txomon commented 9 years ago

I meant that you should run flake8 for the formatting stuff:

$ flake8 plugins/base_commands.py 
plugins/base_commands.py:41:80: E501 line too long (156 > 79 characters)
plugins/base_commands.py:43:80: E501 line too long (126 > 79 characters)
plugins/base_commands.py:44:5: E301 expected 1 blank line, found 0
plugins/base_commands.py:48:80: E501 line too long (100 > 79 characters)
plugins/base_commands.py:50:38: E261 at least two spaces before inline comment
plugins/base_commands.py:50:39: E262 inline comment should start with '# '
plugins/base_commands.py:52:80: E501 line too long (124 > 79 characters)
plugins/base_commands.py:53:5: E301 expected 1 blank line, found 0
JerryHigarr commented 9 years ago

There we go :) - thank you guys so much for being understanding with a beginner :) (for what it matters, flake8 still spits this at me: "unknown option 'application-import-names' ignored ")

Gjum commented 9 years ago

Is ''.join(data['name']) really needed? Same with the other join. What really happens there is you split the string in chars for iteration, and then join the chars to get the original string back.

JerryHigarr commented 9 years ago

Fixed, thanks.

JerryHigarr commented 9 years ago

anything else you need me to fix on this? or is it ready to merge?

txomon commented 9 years ago

I can't accept it (I don't control this part), but here are some more comments on the patch, which are just observations and recommendations, is up to @gamingrobot to then merge it.

In handle_tpa you create args from data, but you just use it [0] from it. Apart from not being clear what 'args' is meant to represent, would it maybe more clear if you created something like:

from_who = data['args'][0]

And later you just have to use it from_who

One more recommendation I usually do is to keep in the first indentation the normal flow, and leave other cases in secondary levels, for example (did this without formatting):

    def handle_tpa(self, event, data):
        if not data['args']:
            self.net.push_packet('PLAY>Chat Message',
                                 {'message': '/tell ' + data['name'] +
                                  ' Usage: (!)tpa [name]'})
            return
        from_who = data['args'][0]
        self.tpa_reqs[from_who] = data['name']
        self.net.push_packet('PLAY>Chat Message',
                             {'message': '/tell %s would like to tpa to you, type (!)tpaccept or (!)tpdeny' % (from_who)})

That way you have a more clear path. Last but not least, I have seen the request, the accept, but I haven't seen the deny =)

gamingrobot commented 9 years ago

Looks good :+1: