Re: [PATCH v3 07/28] git-remote-mediawiki: Rewrite unclear line of instructions
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
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
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
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
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
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
