Alon Bar-Lev has posted comments on this change.
Change subject: Implementation of machine dialog parser for python
......................................................................
Patch Set 1:
(7 comments)
hi,
great work, few major comments...
1. in python unlike java you have flexible dictionaries, so instead of having a
class for each type you can use single dictionary with type key and other keys
based on the actual command.
2. the machine protocol is designed to be easy to parse, so I would like to
avoid regular expression if possible. also, I would expect a single place for
all parsing so even if you are using regular expression a simple array of:
translation = (
{
type='log'
regexp=r'^[*]{3}L:(?P<level>) (<msg>.*)$',
},
...
)
this can easily iterate the dictionary, stop at first match and create
dictionary with the match groups and the type, without extra classes or logic.
http://gerrit.ovirt.org/#/c/28180/1/src/otopi/machine_dialog_parser.py
File src/otopi/machine_dialog_parser.py:
Line 174: head_regex = re.compile('^[*]{3}Q:STRING (?P<name>.*)$')
Line 175:
Line 176: def _response(self):
Line 177: if not isinstance(self.value, basestring) or '\n' in
self.value:
Line 178: msg = "QueryString.value must be single-line string, got:
%s"
please avoid use temp variables
Line 179: raise TypeError(msg % self.value)
Line 180: return self.value
Line 181:
Line 182:
Line 193: """
Line 194: head_regex = re.compile('^[*]{3}Q:MULTI-STRING '
Line 195: '(?P<name>[^ ]+) '
Line 196: '(?P<boundary>[^ ]+) '
Line 197: '(?P<abort_boundary>[^ ]+)$')
please do not draw code, single indent always.
re.compile(
'....'
'...'
)
please use multiline notation, find examples at engine sources.
Line 198:
Line 199: def __init__(self, name, boundary, abort_boundary):
Line 200: super(QueryMultiString, self).__init__(name)
Line 201: self.boundary = boundary
Line 341: class MachineDialogParser(object):
Line 342: """
Line 343: Machine dialog parser.
Line 344: """
Line 345: logger = logging.getLogger(name=constants.Log.LOGGER_BASE)
please inherit from base, so you have logger
Line 346: event_types = (NoteEvent,
Line 347: TerminateEvent,
Line 348: LogEvent,
Line 349: QueryString,
Line 350: QueryMultiString,
Line 351: QueryValue,
Line 352: ConfirmEvent,
Line 353: DisplayValue,
Line 354: DisplayMultiString)
please do not draw code... single indent...
event_types = (
aaa,
bbb,
)
Line 355:
Line 356: def __init__(self, input_=None, output=None, filter_=tuple()):
Line 357: """
Line 358: Keyword arguments:
Line 364: self.output = None
Line 365: self.input_ = None
Line 366: self.set_streams(input_, output)
Line 367: self.filter_ = None
Line 368: self.set_filter(filter_)
not sure what is the xxx_ convention... please use _ prefix for members.
Line 369:
Line 370: def _write(self, data):
Line 371: """
Line 372: Writes data to output stream
Line 378: self.output.write(data)
Line 379: self.output.write('\n')
Line 380: self.output.flush()
Line 381:
Line 382: def set_filter(self, filter_):
why do we need to filter?
Line 383: """
Line 384: Set list of events which should be skipped by parser.
Line 385:
Line 386: Keyword arguments:
Line 411: def next_event(self):
Line 412: """
Line 413: Returns instance of Event
Line 414: """
Line 415: line = self.input_.readline()
please do not assume string streams
Line 416: if not line:
Line 417: raise UnexpectedEOF()
Line 418: for event_type in self.event_types:
Line 419: try:
--
To view, visit http://gerrit.ovirt.org/28180
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia7b715a39840e548ef768ac9e77004cc0d98a9c0
Gerrit-PatchSet: 1
Gerrit-Project: otopi
Gerrit-Branch: master
Gerrit-Owner: Lukas Bednar <[email protected]>
Gerrit-Reviewer: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: [email protected]
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
