[Svnmerge] [PATCH] Indented log patch modified v5

[Giovanni Bajo]

Raman Gupta <rocketraman at fastmail.fm> wrote:


>> Note that I also fixed your indentation, and
>> changed pop(lineslist) to lineslist.pop(). Is pop a recently added
>> function that we should have a local replacement for?

list.pop has always been there, it surely doesn't need a local replacement like
we do for the strip family.


>> +def prefix_lines(prefix, lines):
>> +    """Given a string representing one or more lines of text,
>> insert the +    specified prefix at the beginning of each line, and
>> return the result. +    If the input is terminated with a newline,
>> the result will also be +    terminated with a newline."""
>> +    lineslist = lines.split("\n")
>> +
>> +    newline_at_end = False
>> +    if len(lineslist) > 1 and lineslist[-1] == "":
>> +        # remove unwanted extra element representing the nothingness
>> +        # after the last "\n" in the input string
>> +        lineslist.pop()
>> +        newline_at_end = True
>> +
>> +    prefixed_lines = "\n".join([prefix + L for L in lineslist])
>> +    if newline_at_end:
>> +        prefixed_lines += "\n"
>> +    return prefixed_lines

Uhm I saw this went through several iteration. I'm satisfied with the semantic
of the function (as shown in the unittests), but I think it ought to be
simplified a little. What are the cases in which "lines" does not end with \n?
Since it comes from get_commit_log, I'm reasonably sure it will always contain
a trailing newline. This said:

>>> def prefix_lines(prefix, lines):
...     assert lines[-1] == "\n"    # ambiguous otherwise, is "aa" a line?
...     return prefix + lines[:-1].replace("\n", "\n"+prefix) + "\n"
...
>>>
>>> prefix_lines("zz", "\n")
'zz\n'
>>> prefix_lines("zz", "foo\n")
'zzfoo\n'
>>> prefix_lines("zz", "\nfoo\nbar\n")
'zz\nzzfoo\nzzbar\n'

Then you can call prefix_lines directly with "message". Am I missing something?


>> +        if len(message) > 0:

aka "if message".

>> +            logs.append(prefix_lines(LOG_LINE_PREFIX, \
>> +                                     rstrip(message, "\n")) + "\n")
>> +            for match in LOG_SEPARATOR_RE.findall(message):

Given this prefix, will LOG_SEPARATOR_RE still work? Current definition is:

LOG_SEPARATOR_RE = re.compile('^((%s)+)' % re.escape(LOG_SEPARATOR),
re.MULTILINE)

So it seems it expects the dots to be at the beginning of the line.

Giovanni Bajo