Converting Python Scripts to meet PEP-8 (and bugs)

Announcements and discussion regarding any projects related to Cyan Worlds' Plasma Engine including (but not limited to) CyanWorlds.com Engine, Drizzle, OfflineKI, PyPRP, and libHSPlasma.

Re: Converting Python Scripts to meet PEP-8

Postby johnsojc » Sat Jul 20, 2013 12:25 pm

Tsar Hoikas wrote:I'm slowly working through the commit(s) and adding my comments. There are so many changes that it will probably take me awhile to finish, so I'll post here when I'm done.

I have stopped working on the PEP8 work until I hear back from you (needed a break anyway).
Tsar Hoikas wrote:Hmm... Usually git for windows will checkout CRLF but commit LF line endings. You might have an unexpected setup that is confusing git. It was written by Linux, so expect it to be anal-retentive about things like that ;)

git config core.autocrlf false turns it off

EDIT: OK. I'm running the cleaned up SDL files on both my server and client and all seems well. Will do more testing.
johnsojc
 
Posts: 246
Joined: Sun Dec 07, 2008 10:27 am

Re: Converting Python Scripts to meet PEP-8

Postby johnsojc » Sun Jul 21, 2013 6:20 am

About xAgeSDLBoolShowHide.py:

It appears you can get two kinds of errors trying to get the SDL data...
Index errors or Key errors.
    Going to the city, you get an index error on trying to get nb01ReaderBoardVis.
    Going to Watcher's Pub, you get key error on trying to get boolTreeDayLights
Gobbling blanks in SDL names is still necessary.

Changing xAgeSDLBoolShowHide.py as follows makes the tracebacks go away but probably does not solve the problem (i.e. I have no idea if the default setting leaves the object the way you want it... I will check.)
Code: Select all
        if sdlName.value:
            # So, apparently, Cyan's artists like trailing whitespace...
            if sdlName.value.find(" ") != -1:
                PtDebugPrint("xAgeSDLBoolShowHide._Setup():\tWARNING: %s's SDL variable '%s' has whitespace. Removing!" % (self.sceneobject.getName(), sdlName.value))
                sdlName.value = sdlName.value.replace(" ", "")

            ageSDL.setFlags(sdlName.value, 1, 1)
            ageSDL.sendToClients(sdlName.value)
            ageSDL.setNotify(self.key, sdlName.value, 0.0)
            # Cyan's server will generate some interesting blobs... If this fails, just eat it.
            # It happens because Cyan sucks, and there's nothing we can do about it.
            try:
                self.sdl_value = ageSDL[sdlName.value][0]
            except IndexError:
                self.sdl_value = defaultValue.value
            except KeyError:
                self.sdl_value = defaultValue.value
        else:
            self.sdl_value = defaultValue.value # start at default
            raise RuntimeError("You forgot to set the SDL Variable Name!")

It might be worth setting a PtDebugPrint statement in the excepts so we know something is broken.

Going to check on xAgeSDLBoolRespond now.

EDIT: xAgeSDLBoolRespond produces both key and index errors as well. Ages: Watcher's pub and Er'cana
johnsojc
 
Posts: 246
Joined: Sun Dec 07, 2008 10:27 am

Re: Converting Python Scripts to meet PEP-8

Postby johnsojc » Sun Jul 21, 2013 7:55 am

This works for xAgeSDLBoolRespond.py
Change:
Code: Select all
        # Setup the SDL for notifications
        ageSDL.setFlags(sdlName.value, 1, 1)
        ageSDL.sendToClients(sdlName.value)
        ageSDL.setNotify(self.key, sdlName.value, 0.0)

        # Now do the nasty
        self._Execute(ageSDL[sdlName.value][0], initFastFwd.value)

to:
Code: Select all
        # Setup the SDL for notifications
        ageSDL.setFlags(sdlName.value, 1, 1)
        ageSDL.sendToClients(sdlName.value)
        ageSDL.setNotify(self.key, sdlName.value, 0.0)

        try:
            self._Execute(ageSDL[sdlName.value][0], initFastFwd.value)
        except IndexError:
            self._Execute(defaultValue.value, initFastFwd.value)
        except KeyError:
            self._Execute(defaultValue.value, initFastFwd.value)

        # Now do the nasty
        # self._Execute(ageSDL[sdlName.value][0], initFastFwd.value)
johnsojc
 
Posts: 246
Joined: Sun Dec 07, 2008 10:27 am

Re: Converting Python Scripts to meet PEP-8

Postby Tsar Hoikas » Sun Jul 21, 2013 5:08 pm

I haven't had a chance to look at those errors yet. I just updated all my PCs to Windows 8--when I upgrade, I launch nukes at hard drives and start over ;). I'll create a pull request for you to look over when I do that (hopefully later tonight). I think there's some class relationship between IndexError and KeyError, so we really ought to be able to get away with just catching one of them. A debug print is a good idea as well.

I did however manage to finish looking over your first commit. It was a doozy to read because of all of the whitespace changes, but they were necessary, so that's fine. I expect to be able to finish the next two more quickly. I noticed a few problems that I think we'll want to address moving forward. I think the most major (to me) is the code-breaking up and license block changes introduced by attempting to make the code fit in 79 columns. I know I've said it before, but Uru's code really lends itself to long lines. I usually set my guide markers to 100 columns and try not to go too far past that. In my experience, pep8 checkers tend to explode on things that really don't matter or are so old they don't take into account new features, such as sets.

A few other things:
  • I would prefer the license block not be changed by anyone except Cyan
  • I would prefer to see code on one line, rather than broken up over many.
  • I have a stylistic preference for no parens in single condition if/elif statements.
  • In a few places, you're testing isinstance(foo, type(bar)), this should really be isinstance(foo, bar). The two offenders I remember are bar == None and bar == "".
  • You've changed docstrings (the multiline strings at the beginning of function bodies) into comments. We should leave those as docstrings
  • There are a few places where the code was testing if some variable is equal to a bunch of strings individually. It would be better and faster to test membership in a frozenset (a frozenset is just a set not assigned to any name).
  • Instead of converting print statements to the print_function, I would prefer to use PtDebugPrint. One day, we might decide to implement that function engine side so the in game python log can be colorized :)
  • You went through the effort of fixing commented out code. That's really awesome, but we are using version control. You may delete all the commented out cruft you come across. ;)

I know it's a pain that I'm asking you to revisit a lot of what you've done. I really do appreciate that you're taking the time to make this mess a lot less messy :)

EDIT: and done reading the entire PR!
Last edited by Tsar Hoikas on Sun Jul 21, 2013 5:36 pm, edited 1 time in total.
Reason: timestamp Edit 1
Image
User avatar
Tsar Hoikas
Councilor of Technical Direction
 
Posts: 1898
Joined: Fri Nov 16, 2007 9:45 pm
Location: South Georgia

Re: Converting Python Scripts to meet PEP-8

Postby Zrax » Sun Jul 21, 2013 5:35 pm

For the case of the exceptions, it's worth noting that Python (v2 and v3) allow you to combine them in a single except clause:

Code: Select all
try:
    self._Execute(ageSDL[sdlName.value][0], initFastFwd.value)
except (IndexError, KeyError):
    self._Execute(defaultValue.value, initFastFwd.value)
Last edited by Zrax on Sun Jul 21, 2013 5:38 pm, edited 2 times in total.
User avatar
Zrax
 
Posts: 204
Joined: Fri Sep 28, 2007 5:19 pm
Location: Waist-deep in a conecano

Re: Converting Python Scripts to meet PEP-8

Postby Tsar Hoikas » Sun Jul 21, 2013 5:37 pm

That's true as well. However, it appears that both KeyError and IndexError derive from LookupError, so we should be catching that instead :ugeek:
Image
User avatar
Tsar Hoikas
Councilor of Technical Direction
 
Posts: 1898
Joined: Fri Nov 16, 2007 9:45 pm
Location: South Georgia

Re: Converting Python Scripts to meet PEP-8

Postby johnsojc » Sun Jul 21, 2013 5:58 pm

I never saw LookupError in the traceback, just KeyError and IndexError (tuple out of range or something).

I played all the way through Uru Prime with the above modifications and the only traceback error I saw was when I tried to read Sharpers journal...
johnsojc
 
Posts: 246
Joined: Sun Dec 07, 2008 10:27 am

Re: Converting Python Scripts to meet PEP-8

Postby Tsar Hoikas » Sun Jul 21, 2013 6:02 pm

Right, but KeyError and IndexError are both subclasses of LookupError. So, if we catch LookupError, we'll catch both KeyError and IndexError. I've just written a quick fix that catches it, and the Kadish and Guild Pub tracebacks go away :)
Image
User avatar
Tsar Hoikas
Councilor of Technical Direction
 
Posts: 1898
Joined: Fri Nov 16, 2007 9:45 pm
Location: South Georgia

Re: Converting Python Scripts to meet PEP-8

Postby johnsojc » Sun Jul 21, 2013 6:08 pm

<sheepish grin> yeah, I just looked it up and LookupError catches them both. I'll try it in my setup as well.

Know I know there a 6 builtin exceptions in Python27. I may yet learn how to program in Python.
johnsojc
 
Posts: 246
Joined: Sun Dec 07, 2008 10:27 am

Re: Converting Python Scripts to meet PEP-8

Postby Deledrius » Sun Jul 21, 2013 6:26 pm

While it's not imperative for this big pass through the scripts, I should mention in case you want to include this for future Python3 compatibility: I've been changing the old %-style string formatting with the now preferred format() string function. I know Hoikas doesn't particularly care for it, but it is the way forward in Python.
User avatar
Deledrius
Gehn Shard Admin
 
Posts: 1270
Joined: Mon Oct 01, 2007 1:21 pm

PreviousNext

Return to Plasma Development

Who is online

Users browsing this forum: No registered users and 2 guests

cron