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

From: Célestin Matte <celestin.matte@ensimag.fr>
Date: Jun 10, 2013, 10:48 UTC
Message-ID: <51B5AEE6.8010102@ensimag.fr>
In-Reply-To: <CAPig+cQWQ842-ZG3_JQxZ=-2qLPpijx9z3vtXns+G3xxp6PsyQ@mail.gmail.com>

Le 10/06/2013 02:50, Eric Sunshine a écrit :

Show 5 quoted lines

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

Show 8 quoted lines

> 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 :)

Show 19 quoted lines

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