Re: [PATCH v3 22/28] git-remote-mediawiki: Modify strings for a better coding-style

["Célestin Matte" <celestin.matte@ensimag.fr>]

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