Le 10/06/2013 02:50, Eric Sunshine a écrit :
> Given this patch's intention to use ${} within strings, should this be
> ${credential{username}}?
>
> (I don't have a preference, but it's a genuine question since it's not
> clear if this was an oversight or intentional.)
The answer is simple: I didn't know the exact syntax, so I didn't bother
doing it.
> The whitespace-only change to line "my $res = do {" is effectively
> noise. The reviewer has to stop and puzzle out what changed on the
> line before continuing with review of the remaining _real_ changes. It
> is a good idea to avoid noise changes if possible.
>
> In this particular case, it's easy to avoid the noise since the
> trailing space on that line could/should have been removed in patch
> 18/28 when the statement was split over multiple lines.
Actually, I noticed this but didn't find what the difference between the
two lines was. I assumed git was making some kind of mistake - but eh,
it seems git is never wrong :)
>> local $/ = undef;
>> <$git>
>> };
>> @@ -475,26 +475,26 @@ sub download_mw_mediafile {
>> return $response->decoded_content;
>> } else {
>> print STDERR "Error downloading mediafile from :\n";
>> - print STDERR "URL: $download_url\n";
>> - print STDERR "Server response: " . $response->code . " " . $response->message . "\n";
>> + print STDERR "URL: ${download_url}\n";
>> + print STDERR 'Server response: ' . $response->code . q{ } . $response->message . "\n";
>
> To meet the goals of this patch, would you want to do this instead?
>
> "Server response: @{[$response->code]} @{[$response->message]}\n";
>
> Whether this is easier or more difficult to read is a matter of
> opinion. (Again, this is a genuine question rather than a show of
> preference on my part.)
Same as above, I tried to change it but didn't know the exact syntax, so
I gave up.
--
Célestin Matte
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html