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

Thanks Paul for looking into it!

On Wed, Dec 4, 2013 at 2:25 PM, Paul Sokolovsky
<email address hidden> wrote:
> Review: Needs Fixing
>
> 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?

I added info in the function doc string.

>
> 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.

I know about the hardcoded @linaro.org values (there are a few more in
Linaro patchwork code), but I'm not going to fix them here. The code
cannot go in production like that, I'm aware of it, that's why I would
like the other MP in first, to be able to address this issue as well.
The other MP, that is bigger, adds some integration with Crowd that we
can use here as well, that's why there is that TODO.

I do not like the regexp approach, at least in this case: we can do
even better than that, and it is taking data out of Crowd.

The problem cannot be solved by simply querying Crowd though:
switching to a subscription model for patches.l.o (we subscribe to the
mailing list, they do not send the patches directly anymore), it means
that we can receive even more than 500 mails per day, that will lead
to 500 Crowd requests. When I tested the old patch, IT were already
looking at who was hitting the server (and it was with far less
requests). We need to locally cache those requests: the other MP tries
to solve that.

> 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...

There was one "return 0" too much, fixed.

--
Milo Casagrande | Automation Engineer
Linaro.org <www.linaro.org> │ Open source software for ARM SoCs