Code review comment for lp:~milo/linaro-patchmetrics/parsemail-fixes

Revision history for this message
Paul Sokolovsky (pfalcon) wrote :

Logic looks good. And I usually don't go at such details with stylistic changes, but for few quick looks at linaro-patchmetrics codebase, I felt confused and saw few less-than-ideal practices and hardcodings. So, let's try to do better.

1. Can "new_author" var be named "is_new_auhor"?

2.

8 - return None, None
9 + return None, None, None

From the rest of patch it's possible to figure out what 3rd return value is, and I'm not sure if that func had a docstring, but maybe it might?

3.

57 + # If submitter nor author of a patch are not Linaro, we move on.
58 + # TODO: may be worth to integrate with crowd.
59 + if "@linaro.org" not in submitter.email:
60 + if author is not None and "@linaro.org" not in author.email:
61 + return 0
62 + return 0

There're 2 issues here: one is that hardcoded "@linaro.org". We can do better ;-) (yeah, I know it takes much more patch lines to use config settings, and sometimes with my changes I wonder if I should bother either; friendly answer looking from outside - yes ;-) ). And we should anticipate need to support multiple patterns - either as array, or better just as a regexp.

Another is that there's superfluous/erroneous logic - if first "if" fired, it's "return 0" regardless of second
"if". If code should look like that, there should be a comment (like, "one sweet day, we may want to handle these conditions separately"). Otherwise...

review: Needs Fixing

« Back to merge proposal