Change in vdsm[master]: v2v: introduction to OutputParser

nsoffer at redhat.com nsoffer at redhat.com
Mon Mar 30 15:20:18 UTC 2015


Nir Soffer has posted comments on this change.

Change subject: v2v: introduction to OutputParser
......................................................................


Patch Set 1: Code-Review+1

(3 comments)

Looks good, can be improved a little bit by separating iteration and parsing code.

https://gerrit.ovirt.org/#/c/39360/1/tests/v2vTests.py
File tests/v2vTests.py:

Line 24
Line 25
Line 26
Line 27
Line 28
Why delete the previous tests?


https://gerrit.ovirt.org/#/c/39360/1/vdsm/v2v.py
File vdsm/v2v.py:

Line 91:                 description, current_disk, disk_count = self._parse_line(line)
Line 92:                 yield ImportProgress(int(current_disk), int(disk_count),
Line 93:                                      description)
Line 94:                 for progress in self._parse_progress(stream):
Line 95:                     yield DiskProgress(progress)
If you use my suggestions bellow:

    for chunk in self._iter_progress(stream):
        progress = self._parse_progress(chunk)
        yield DiskProgress(progress)
        if progress == 100:
            break

This keep all the flow logic in one place, and keep the parsing code away.
Line 96: 
Line 97:     def _parse_line(self, line):
Line 98:         m = self.COPY_DISK_RE.match(line)
Line 99:         if m is None:


Line 116:                 try:
Line 117:                     progress = int(m.group(1))
Line 118:                 except ValueError:
Line 119:                     raise OutputParserError('error parsing progress regex: %r'
Line 120:                                             % m.groups)
This is little bit too long, and is doing two things - extracting chunks terminated by "\r", and parsing the content of the chunks.

This block can be extracted to _parse_progress() (like _parse_line).

Then the rest can renamed to _iter_progress(). Will be easier to follow.
Line 121:                 yield progress
Line 122: 
Line 123: 
Line 124: def _mem_to_mib(size, unit):


-- 
To view, visit https://gerrit.ovirt.org/39360
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I54f3d610d704d0b922e38073edcc813cbc27fe83
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shavivi at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list