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

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

quoted

                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