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

[johnsojc]

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).

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]

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]

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)

[Tsar Hoikas]

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!

[Zrax]

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)

[Tsar Hoikas]

That's true as well. However, it appears that both KeyError and IndexError derive from LookupError, so we should be catching that instead

[johnsojc]

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...

[Tsar Hoikas]

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

[johnsojc]

<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.

[Deledrius]

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.