Lukas Bednar has posted comments on this change.
Change subject: Implementation of machine dialog parser for python
......................................................................
Patch Set 1:
(7 comments)
Thanks for quick review.
regarding to dict vs class, I don't agree that it can be done without
additional logic. here I put that logic into classes. in
MachineDialogParser.java there is _parseRequest which contains big if-else
tree, where the similar logic is implemented as well.
there are also differences in responses (including abort), which is controlled
by another tree of if-else in java implementation. it seems to me more suitable
to encapsulate these differences (including attributes) in class instead.
regarding to regular expressions. I don't mind I can replace it by
line.startswith(XXX), line.split(' ', Y).
I decided to use regex from single reason: I am using named_groups
'(?P<name>xxxx)', where the 'xxxx' is name of variable in Event constructor. it
is similar to DataBoundConstructor annotation which is used in 'stapler'
project, and it came handy to me here.
I would like to include test for that parser. I can not find the place in this
repository where I could put tests for these python modules.
could you please advise me how to include that ?
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
Done
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.
Done
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
Done
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...
ok
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.
'filter' is build-in function, same as 'input', 'id', 'type', ...
I am avoiding to hiding them in local scope.
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?
I was thinking about ignoring events by parser. for example in case I don't
care about NoteEvent and LogEvent, I would pass filter_=(NoteEvent, LogEvent).
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
what should I assume ? file descriptor ?
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 <lbed...@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com>
Gerrit-Reviewer: Lukas Bednar <lbed...@redhat.com>
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
