Re: [PATCH v3 07/28] git-remote-mediawiki: Rewrite unclear line of instructions

2013-06-11 Thread Junio C Hamano
Célestin Matte  writes:

> Le 11/06/2013 20:09, Junio C Hamano a écrit :
>> Matthieu Moy  writes:
>> 
   my ($namespace) = @_;
my $namespace = shift;

 My impression has been that both are equally common,
>>>
>>> The second is the most common in git-remote-mediawiki (but I don't have
>>> any preference nor know what is recommended elsewhere).
>> 
>> I wasn't implying I prefer the former.  I was just being curious,
>> and if the latter is more prevalent in the code _and_ Perlcritique
>> does not have any issue with it, it is perfectly fine.
>
> Perlcritic doesn't have an issue with any of both cases.

OK.  As this topic is about matching the opinion of Perlcritique, I
think it is fine either way (which was the primary thing that I
wanted to find out).

> I think the second method is clearer when there is only one argument,
> because it makes it clear that there is only one.

Hmm, from the maintenance point of view, the second one invites the
next person to extend this function like this:

my $namespace = shift;
+   my $newargument = shift;
+   my $anotherargument = shift;

If your original were in the first style, instead you would likely to
get this:

-   my ($namespace) = @_;
+   my ($namespace, $newargument, $anotherargument) = @_;

When there is only one argument, it is clear that there is only one
argument in either style.  It is not a strong factor to pick one
style over the other.  Once you start taking more than one argument,
however, I think "it makes it clear what arguments the function
takes" would actually favor the style to split @_ into a list of
local variables.

But as I said earlier, this patch is about following Perlcritique's
advice, and because it does not have opinion on these styles, it is
outside the scope of this patch.

Thanks for checking.
--
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


Re: [PATCH v3 07/28] git-remote-mediawiki: Rewrite unclear line of instructions

2013-06-11 Thread Célestin Matte
Le 11/06/2013 20:09, Junio C Hamano a écrit :
> Matthieu Moy  writes:
> 
>>>   my ($namespace) = @_;
>>> my $namespace = shift;
>>>
>>> My impression has been that both are equally common,
>>
>> The second is the most common in git-remote-mediawiki (but I don't have
>> any preference nor know what is recommended elsewhere).
> 
> I wasn't implying I prefer the former.  I was just being curious,
> and if the latter is more prevalent in the code _and_ Perlcritique
> does not have any issue with it, it is perfectly fine.

Perlcritic doesn't have an issue with any of both cases.
I think the second method is clearer when there is only one argument,
because it makes it clear that there is only one.

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


Re: [PATCH v3 07/28] git-remote-mediawiki: Rewrite unclear line of instructions

2013-06-11 Thread Junio C Hamano
Matthieu Moy  writes:

>>   my ($namespace) = @_;
>>  my $namespace = shift;
>>
>> My impression has been that both are equally common,
>
> The second is the most common in git-remote-mediawiki (but I don't have
> any preference nor know what is recommended elsewhere).

I wasn't implying I prefer the former.  I was just being curious,
and if the latter is more prevalent in the code _and_ Perlcritique
does not have any issue with it, it is perfectly fine.

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


Re: [PATCH v3 07/28] git-remote-mediawiki: Rewrite unclear line of instructions

2013-06-11 Thread Matthieu Moy
Junio C Hamano  writes:

> Célestin Matte  writes:
>
>> Subroutines' parameters should be affected to variable before doing anything
>> else
>> Besides, existing instruction affected a variable inside a "if", which break
>> Git's coding style
>
> I think s/affect/assign/g is what you meant.

Yes, common false friends for French people ;-).

>   my ($namespace) = @_;
>   my $namespace = shift;
>
> My impression has been that both are equally common,

The second is the most common in git-remote-mediawiki (but I don't have
any preference nor know what is recommended elsewhere).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
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


Re: [PATCH v3 07/28] git-remote-mediawiki: Rewrite unclear line of instructions

2013-06-11 Thread Junio C Hamano
Célestin Matte  writes:

> Subroutines' parameters should be affected to variable before doing anything
> else
> Besides, existing instruction affected a variable inside a "if", which break
> Git's coding style

I think s/affect/assign/g is what you meant.

By the way, I often see two styles of the "let's take arguments into
parameters before doing anything else" at the beginning of subs:

my ($namespace) = @_;
my $namespace = shift;

My impression has been that both are equally common, but the latter
is done more often when picking out small and fixed number of
mandatory parameters upfront (and later, optional parameters are
used by directly reading what remains in @_).  Does Perlcritique say
anything about this issue?

> Signed-off-by: Célestin Matte 
> Signed-off-by: Matthieu Moy 
> ---
>  contrib/mw-to-git/git-remote-mediawiki.perl |3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl 
> b/contrib/mw-to-git/git-remote-mediawiki.perl
> index 431e063..2db6467 100755
> --- a/contrib/mw-to-git/git-remote-mediawiki.perl
> +++ b/contrib/mw-to-git/git-remote-mediawiki.perl
> @@ -1333,7 +1333,8 @@ sub get_mw_namespace_id {
>  }
>  
>  sub get_mw_namespace_id_for_page {
> - if (my ($namespace) = $_[0] =~ /^([^:]*):/) {
> + my $namespace = shift;
> + if ($namespace =~ /^([^:]*):/) {
>   return get_mw_namespace_id($namespace);
>   } else {
>   return;
--
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


[PATCH v3 07/28] git-remote-mediawiki: Rewrite unclear line of instructions

2013-06-09 Thread Célestin Matte
Subroutines' parameters should be affected to variable before doing anything
else
Besides, existing instruction affected a variable inside a "if", which break
Git's coding style

Signed-off-by: Célestin Matte 
Signed-off-by: Matthieu Moy 
---
 contrib/mw-to-git/git-remote-mediawiki.perl |3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl 
b/contrib/mw-to-git/git-remote-mediawiki.perl
index 431e063..2db6467 100755
--- a/contrib/mw-to-git/git-remote-mediawiki.perl
+++ b/contrib/mw-to-git/git-remote-mediawiki.perl
@@ -1333,7 +1333,8 @@ sub get_mw_namespace_id {
 }
 
 sub get_mw_namespace_id_for_page {
-   if (my ($namespace) = $_[0] =~ /^([^:]*):/) {
+   my $namespace = shift;
+   if ($namespace =~ /^([^:]*):/) {
return get_mw_namespace_id($namespace);
} else {
return;
-- 
1.7.9.5

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