UniversalDevicesInc / Polyglot

Polyglot Open Source
MIT License
24 stars 2 forks source link

Added documentation for #1 and some for #18. #44

Closed jimboca closed 8 years ago

jimboca commented 8 years ago

Added setup method for SimpleNodeServer to define the logger and allow for future additions when necessary.

The generated html creates a big diff, so it can be ignored. Please review the changes in nodesever_api.py

jimboca commented 8 years ago

@mjwesterhof @Einstein42 If you guys are ok with these changes I will merge into development.

mjwesterhof commented 8 years ago

I see something about a new requirement about calling the super when creating a new node - will this break existing node server code? To play the "devil's advocate" role here, the need to call the super class init isn't what I'd consider to be "simple" python at all - so does this result in the simple node server needing to be renamed to not-quite-so-simpleNodeServer instead? :-D (Seriously, has the addition of the logger complicated life for beginning Python developers?)

mjwesterhof commented 8 years ago

Also, do we need to protect references to "logger" with a null check? What if the params message doesn't arrive - won't the node server crash?

jimboca commented 8 years ago

The need to call super is for the SimpleNodeServer setup and will not break any existing node servers/ IMO this is the best way to alow us to add new features without requiring changes to the NodeServer itself, and is a standard Python OO way to properly handle method inheritance when you don't want to override everything, just like we already call super in the Node init methods.

These changes will only break node servers that already use .LOGGER since Michel agreed this shouldn't be uppercase and has been changed to .logger.

The change for node servers already using the logger is from this:

class CameraNodeServer(SimpleNodeServer):
    """ ISY Helper Camera Node Server """

    def setup(self):
        self.logger = self.poly.LOGGER
        self.logger.info('CameraNodeServer starting up.')
class CameraNodeServer(SimpleNodeServer):
    """ ISY Helper Camera Node Server """

    def setup(self):
        super(SimpleNodeServer, self).setup()
        self.logger.info('CameraNodeServer starting up.')

I don't know how to progect references to logger, but IMO if the params message doesn't arrive the node server will crash for many reasons?

mjwesterhof commented 8 years ago

I'd say to go ahead and merge this into the development branch -- it's a development branch, after all! If something breaks, we'll figure it out and fix it. Integrate early, integrate often.

Regarding the logger and null check -- I'm referring to the couple of places in Polyglot that attempt to use it. I've looked into that a bit deeper, and I'd like to generally strengthen the error logging and checking during the node server startup code. I'll update that post-merger of this into the dev branch. I know it sounds that I'm over-concerned, but in my defense, I'll just say that decades of release engineering, QA, and customer support work have left me scarred for life. And Java/Python stack traces as substitutes for error messages make me pull out what little hair I have left. :)

jimboca commented 8 years ago

Changes are merged.

Regarding the logger null check, I do pretty much agree with you, I really don't like that it seems to be acceptable behavior for Python to just show stack traces. I try to add error checking where it seems appropriate But for the logger null check, won't the server die sooner if the params never come back?