Re: [PATCH v3 07/28] git-remote-mediawiki: Rewrite unclear line of instructions
Célestin Matte celestin.ma...@ensimag.fr 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 celestin.ma...@ensimag.fr
Signed-off-by: Matthieu Moy matthieu@grenoble-inp.fr
---
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
Re: [PATCH v3 07/28] git-remote-mediawiki: Rewrite unclear line of instructions
Matthieu Moy matthieu@grenoble-inp.fr 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
Le 11/06/2013 20:09, Junio C Hamano a écrit : Matthieu Moy matthieu@grenoble-inp.fr 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
Célestin Matte celestin.ma...@ensimag.fr writes: Le 11/06/2013 20:09, Junio C Hamano a écrit : Matthieu Moy matthieu@grenoble-inp.fr 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
[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 celestin.ma...@ensimag.fr
Signed-off-by: Matthieu Moy matthieu@grenoble-inp.fr
---
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
