Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Add protocol schema handling #3

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

janLo
Copy link

@janLo janLo commented Jan 9, 2017

This tries to add protocol schema verification (basically checking/conversion of datatypes) for the protocol data sent to the daemon. In the first version it uses a static protocol schema dump generated from a patched pilight). The code to generate this is provided in [this PR](pilight/pilight#322.

As soon as this is merged we can detect the new feature from the pilight version and query it from the daemon itself.

This is a schema definition that was generated by the code from the
code in pilight/pilight#323. It is used until pilight merge it and give
an option to get the actual schma of the running daemon.

Signed-off-by: Jan Losinski <losinski@wh2.tu-dresden.de>
This implements a protocol schema registry that is filled by the
pilight protocol schema definition. It provides and easy mechanism to
validate protocol data and convert arguments to the correct datatypes.

Signed-off-by: Jan Losinski <losinski@wh2.tu-dresden.de>
In this first version it uses only the static default protocol schema.
As soon as pilight provides a mechanism to get this data from the
daemon itself, we can replace this or make the source of the schema
dependant from the daemon version.

Signed-off-by: Jan Losinski <losinski@wh2.tu-dresden.de>
@janLo
Copy link
Author

janLo commented Jan 9, 2017

@DavidLP Even if the tests are failing and it is all still WIP I would appreciate to get some early feedback from you :)

Signed-off-by: Jan Losinski <losinski@wh2.tu-dresden.de>
Signed-off-by: Jan Losinski <losinski@wh2.tu-dresden.de>
Signed-off-by: Jan Losinski <losinski@wh2.tu-dresden.de>
@coveralls
Copy link

Coverage Status

Coverage decreased (-9.09%) to 90.909% when pulling 551782e on janLo:protocol_list into e111eb5 on DavidLP:master.

@@ -103,6 +105,11 @@ def __init__(self, host='127.0.0.1', port=5000, timeout=1,
answer_1, answer_2)

self.callback = None
self.protocol_registry = self._load_protocol_schema()

def _load_protocol_schema(self):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a essential function to be used externally I would remove the _ prefix.

@@ -0,0 +1,2638 @@
{
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is generated from development? We could incorporate a protocol version (e.g. release 7). But it is not that important.

self.schema)


class ProtocolRegistry(object):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not get, why a class with validate is needed. This is what vol provides no?. We can just create a vol.Schema from the json dump.
Also can you add an example how to use the protocol validation schema?

@DavidLP
Copy link
Owner

DavidLP commented Jan 21, 2017

I am generally fine with the changes, and have some minor comments. For sure we have to add unit tests and bump the coverage to 100% again ;-). Also I would like to know how to use this shown in an example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants