Summary suggestion:
"Parse out testing_done field from commit messages"

Can you re-flow this as:

result = {
    'key': 'value',
}

How about: "Determine if the commit message contains testing done information."

You can add:

only_testing_done = i == 1


and remove the computation for it below. This will be a cheaper computation.
We will also need to set it to False before in the else statement.

Comments should never have first-person statements. They are not the justification for your changes -- the commit message is.
You can explain something is, but not why something changed. An explanation of why something changed doesn't make sense when you're looking at a file -- it only makes sense when you're looking at the commit as a whole. And when you're doing that, you can read the commit message for justification.

These sorts of comments are unnecessary because they can be inferred from the if statements.

Comments should be full sentences: they should begin with a capital letter and end with a period.

Missing module-level docstring.

Docstring should be of the format: "Unit tests for rbtools.clients.SCMClient"

Docstring should be of the form

"""Testing SCMClient.get_commit_message when ..."""


If it goes over 79 characters and wraps, the trailing """ should be on its own line.
Same below.

ALL_CAPS is reserved for module-level and class-level constants. This would be fine as commit_message

Single triple-quotes ('''). Double quotes are reserved for docstrings only.
Same below.

Only module-level and class-level constnats should be ALL_CAPS. This is fine as commit_message.

We're using these immediately, so I don't know that this is necessary.
Same below.

We are doing this in every test. We can do:

def setUp(self):
    super(SCMBaseClientTests, self).setUp()

    self.client = SCMClient()


and use self.client in all the tests. We like to keep all common set-up / teardown logic in these methods. See unittest.TestCase docs for information about setUp, etc.

Instead of this, how about:

self.assertEqual(
    client.get_commit_message([]),
    {
        'summary': lines[0],
        'description': '',
        'testing_done': lines[3],
    })


That way we test all three at once and test that there are no other keys returned.
Same below.

"commit message"
Missing a period.

"commit message"
Missing a period.

Is this still a todo?

Undo this.

"summary, description, or testing done"

instead of saying the field names over again, how about:
"override the options for each guessed field" (in lieu of "get the ... options")

Leftover debugging? :)

We're doing this thrice, so we should probably refactor this into a method.

"Return" over get.
Can you add args/ returns to this docstring?
On second thought, you may want to hold off on these docstring changes since I believe David has a pending change that updates all docstrings in RBTools to bring them up to spec. Once that lands, you can rebase off of it.

``summary``, ``description``, and ``testing_done``

:py:class:`difflib.SequenceMatcher`

:py:class:`Score`

I assume {field}_pair is a tuple, so can we do:

summary_score = SequenceMatcher(None, *summary_pair).ratio()
description_score = SequenceMatcher(None, *description_pair).ratio()
testing_done_score = SequenceMatcher(None, *testing_done_pair).ratio()


Ideally, we could also do this in a dict comprehension and splat into the Score intializer:

raw_scores = {
    field_name: SequenceMatcher(None, *field_pair).ratio()
    for field_name, field_pair in (
        ('summary_score', summary_pair),
        ('description_score', description_pair),
        ('testing_done_score', testing_done_pair),
    )
}

return Score(**raw_scores)

This needs to be updated with args, returns, etc but needs to be based of David's docstring change.

Leftover testing code? :)

Can we format this as:

result = {
    'summary': lines[0],
    'description': b'',
    'testing_done': b'',
}

Blnank line between these.

This comment is redundant.

This needs to be in the if r.match(line) block above.

Instead of doing .strip() on each line, how about we remove those calls and do:

return {
    key: value.strip()
    for key, value in six.iteritems(result)
}

We've already set summary.

We've already set summary.

We've already set summary.

We've already set summary.

Module docstrings need to be first, e.g.

"""Unit tests for rbtools.clients.SCMClient."""

from __future__ import unicode_literals

# ....

No period.

No period.

No period.

No period.

No period.

No period.

No period.

Mind adding a comment same as above to indicate what this is doing? Also those comments are missing periods, could you add them?

No blank line here.

Leftover debugging?

Docstring summaries should be written in the imperitive mood (i.e., they give an order or command). In this case, it would be "Check" over "Checks", e.g.

Check if the guess value for a given field is True or False


However, we could also write this as:


Determine whether or not we should guess a given field.


Also missing args/returns.

Can you revert changes to this file? See my comment in rbtools/utils/review_request.py for the reasoning.

Can you revert the changes to this file? I don't think we want to match on testing done, since a LOT of changes have "ran unit tests" as testing done, we would get lots of false positives.

Revert this too.

Can you write your testing done without "I" statements, e.g.
"Ran unit tests."

"Created a test repository..."

"is description and [i:] is testing done"

This is unnecessary with the above assignment.

We are doing this when we assign result above.

WIth the above comment, this would be elif not summary

Docstrings should be in the following format:

"""Single line summary.

Multi-line description.
"""


This can just be

"""Unit tests for rbtools.clients.SCMClient."""

Blank line between these.

unicode.

Missing type: bool

bool

) on previous line

) on previous line

) on previous line

Instead of enumerating all these cases, how about:

                    if guess_summary:
                        self.options.summary = guessed_summary

                    if guess_description:
                        if not guess_summary:
                            # If we're guessing the description but not the summary
                            # (for example, if --summary was included), we probably
                            # don't want to strip off the summary line of the
                            # commit message.
                            if guessed_description.startswith(guessed_summary):
                                self.options.description = guessed_description
                            else:
                                self.options.description = \
                                    guessed_summary + '\n\n' + guessed_description
                        else:
                            self.options.description = guessed_description

                    if guess_testing_done:
                        self.options.testing_done = guessed_testing_done

Can you revert all changes to this file?

git checkout master -- rbtools/utils/match_score.py

Can you revert all changes to this file?

git checkout master -- rbtools/utils/review_request.py

Just two comments. This is looking really good! :)

We can optimize this to only check for a testing done when we have a description. We also don't need to enumerate all cases if we do the work of parsing out the testing done when we find it.

if len(lines) >= 3 and lines[0] and not lines[1]:
    # We have a description. This may also contain a testing done.
    description = lines[2:]

    for i, line in enumerate(description):
        if r.match(line):
            result['testing_done'] = '\n'.join(description[i:])
            description = description[:i - 1]

    result['description'] = '\n'.join(description)


This code will be a lot easier to follow and understand.

Why is this lines[0:] now? This used to be lines[1:]. As it stands, i doesn't match up with the actual line number so tests should be failing.

# We have a clear "Summary\n\nDescription"-style commit message so we can split out the description.

What if len(lines) == 2 and lines[1] is not empty, e.g.

This is a summary
this is a description


This should end up with

result = {
    'description': "This is a summary.\nThis is a description.',
    'summary': 'This is a summary.',
    'testing_done': '',
}

Commends should be a full sentence, beginning with a capital letter and ending with a period.

# We do not have a clear "Summary\n\nDescription"-style commit message so we assume everyting is the description.

What about the case of a commit message like:

Testing done:
foo

What happens in the case of a commit message like:

A commit message

Testing done:
Testing done:
I did tests

Your code below does enumerate on description. However, it is a blank string in the case where we only have summary and our loop expects a list!
I suggest we do description = [] here and do the following on lines 298--304:

if description:
    for i, line in enumerate(description):
        if r.match(line):
            # ...
            break

    result['description'] = '\n'.join(description)


This way we skip the case where description is a

We should break here.

"SCMself.client"?

"SCMself.client"?

"SCMself.client"?

"SCMself.client"?

"SCMself.client"?

"SCMself.client"?

No period.

"a summary."

I know I suggested the max() solution, but let's make this more clear by doing the following:

if i == 0:
    # The entire description is the testing done, so
    # the resulting description will be empty.
    description = []
else:
    description = description[:i - 1]

, after description

Re-flow these to take full advantage of the 79 character limit.

Let's call this field_value

"Whether or not we are posting a new review request."

Whether or not the given field should be guessed.

Ship It!

Since this method is being modified with new results, the documentation should be updated to use the modern docstring format. You'd need to make the following changes:

Change "Returns" to "Return" in the summary (we're moving to that style everywhere else).
Add an "Args" section detailing what revisions is.
Add a "Returns" section detailing what's in the dictionary and what string types to expect.

See https://www.notion.so/reviewboard/Writing-Codebase-Documentation-e16312b5f061437cb73cbfa369ac3cb5

I don't believe b'' is correct. We're working with Unicode strings everywhere else, and setting them when we set result['description'] below.

This should go in the second if block below, instead of pass. We don't need to default it to anything above.

Blank line between these.

Blank line between these.

I'd rather we explicitly strip as we set above, and just return result here. That way, we're not doing any unnecessary rebuilding of a dictionary, and we're very clear with what's going into it.

These should all be test_get_commit_message_with_.... The get_ part is missing from the test names.

The dedenting and alignment and everything makes this a bit less ideal for readability. Can you change all these to this form:

commit_message = (
    b'This is the summary.\n'
    b'\n'
    b'Testing Done:\n'
    b'....\n'
)


That may seem more "noisy" (the b prefixes and \n), but with unit tests it's very important to clearly see the data you're working with, and so it's an advantage in this case. We know where every newline is and where the start of every line is, and code doesn't start to run into other code below it due to the indentation.