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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to