Code review comment for ~ycheng-twn/checkbox-ng:0001_checkbox_cli_show

Revision history for this message
Maciej Kisielewski (kissiel) wrote :

(previous comment deleted as LP deleted indentation in the code)

My knee-jerk reaction for re-reading file is to say it's inefficient and we shouldn't do it, but then, we only have to do it once (and one file) per listed unit. This means that when listing one unit, only one extra file open is done, which is bearable. In other words, you're absolutely right, at this stage it's the cleanest solution.
Because there's a concept of 'virtual' units, possibly not containing origin, we must be catch the potential problems stemming from that, so I propose the print to look something like this:

def _print_obj(self, obj):
    try:
        path, line_range = obj.attrs['2origin'].rsplit(':', maxsplit=1)
        start_index, end_index = [int(i) for i in line_range.split('-')]
        with open(path, 'rt', encoding='utf-8') as pxu:
            # origin uses human-like numbering (starts with 1), so we need
            # to substract 1. The range in origin is inclusive,
            # so the end_index is right
            record = pxu.readlines()[start_index - 1 : end_index]
            print(''.join(record))
    except (ValueError, KeyError):
        print("Could not read the record for {}!".format(obj.attrs['id']))
    except OSError as exc:
        print("Could not read '{}' containing record for '{}'!".format(
            path, obj.attrs['id']))

PS Shame that `attrs` is so limited for templates (broken?:)).

« Back to merge proposal