Re: [PATCH] send-email: extract email-parsing code into a subroutine
- From
Matthieu Moy <matthieu.moy@univ-lyon1.fr>
- Date
- Dec 14, 2017, 14:05 UTC
- Message-ID
- <285414621.1144481.1513260333115.JavaMail.zimbra@inria.fr>
- In-Reply-To
- <ce70816f94c24754bea9bc8175de4bc4@BPMBX2013-01.univ-lyon1.fr>
"PAYRE NATHAN p1508475" <nathan.payre@etu.univ-lyon1.fr> wrote:
> - print $c2 $_; > } > + > close $c;
Nit: this added newline does not seem necessary to me. Nothing serious, but this kind of thing tend to distract the reader when reviewing the patch.
> + foreach my $key (keys %parsed_email) {
> + next if $key == 'body';
> + print $c2 "$key: $parsed_email{$key}";
> + }I'd add a comment like
# Preserve unknown headers
at the top of the loop to make it clear what we're doing.
On a side note: there's no comment in the code you're adding. This is not necessarily a bad thing (beautifully written code does not need comments to be readable), but you may re-read your code with the question "did I explain everything well-enough?" in mind. The loop above is a case where IMHO a short and sweet comment helps the reader.
Two potential issues not mentionned in your message but that we discussed offlist is that 1) this doesn't preserve the order, and 2) this strips duplicate headers. I believe this is not a problem here, and trying to solve these points would make the code overkill, but this would really deserve being mentionned in the commit message. First, so that people reviewing your patch now can confirm (or not) that you are taking the right decision by doing this, and also for people in the future examining your patch (e.g. after a bisect).
> +sub parse_header_line {
> + my $lines = shift;
> + my $parsed_line = shift;
> + my $pattern = join "|", qw(To Cc Bcc);Nit: you may want to rename it to something more explicit, like $addr_headers_pat.
None of my nit should block the patch inclusion, but I think the commit message should be expanded to include a mention of the "duplicate headers"/"header order" potential issue.
-- Matthieu Moy https://matthieu-moy.fr/