Re: [PATCH 05/18] Turn double-negated expressions into simple expressions
Le 07/06/2013 06:12, Eric Sunshine a écrit :
On Thu, Jun 6, 2013 at 3:34 PM, Célestin Matte
celestin.ma...@ensimag.fr wrote:
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 |8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl
b/contrib/mw-to-git/git-remote-mediawiki.perl
index 68fd129..a6c7de2 100755
--- a/contrib/mw-to-git/git-remote-mediawiki.perl
+++ b/contrib/mw-to-git/git-remote-mediawiki.perl
@@ -136,16 +136,16 @@ while (STDIN) {
if (defined($cmd[0])) {
# Line not blank
if ($cmd[0] eq capabilities) {
- die(Too many arguments for capabilities\n) unless
(!defined($cmd[1]));
+ die(Too many arguments for capabilities\n) if
(defined($cmd[1]));
mw_capabilities();
} elsif ($cmd[0] eq list) {
- die(Too many arguments for list\n) unless
(!defined($cmd[2]));
+ die(Too many arguments for list\n) if
(defined($cmd[2]));
mw_list($cmd[1]);
} elsif ($cmd[0] eq import) {
- die(Invalid arguments for import\n) unless
($cmd[1] ne !defined($cmd[2]));
+ die(Invalid arguments for import\n) if ($cmd[1] eq
|| defined($cmd[2]));
mw_import($cmd[1]);
} elsif ($cmd[0] eq option) {
- die(Too many arguments for option\n) unless
($cmd[1] ne $cmd[2] ne !defined($cmd[3]));
+ die(Too many arguments for option\n) if ($cmd[1]
eq || $cmd[2] eq || defined($cmd[3]));
Not new in this patch, but isn't this diagnostic misleading? It will
(falsely) claim too many arguments if $cmd[1] or $cmd[2] is an empty
string. Perhaps it should be reworded like the 'import' diagnostic and
say Invalid arguments for option.
We could even be more precise and separate the cases, i.e., die(Too
many arguments) when too many arguments are defined and die(Invalid
arguments) when there are empty strings.
Not sure if I should integrate it in this patch, though.
--
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 05/18] Turn double-negated expressions into simple expressions
On Fri, Jun 7, 2013 at 1:04 PM, Célestin Matte
celestin.ma...@ensimag.fr wrote:
Le 07/06/2013 06:12, Eric Sunshine a écrit :
On Thu, Jun 6, 2013 at 3:34 PM, Célestin Matte
celestin.ma...@ensimag.fr wrote:
} elsif ($cmd[0] eq import) {
- die(Invalid arguments for import\n) unless
($cmd[1] ne !defined($cmd[2]));
+ die(Invalid arguments for import\n) if ($cmd[1]
eq || defined($cmd[2]));
mw_import($cmd[1]);
} elsif ($cmd[0] eq option) {
- die(Too many arguments for option\n) unless
($cmd[1] ne $cmd[2] ne !defined($cmd[3]));
+ die(Too many arguments for option\n) if ($cmd[1]
eq || $cmd[2] eq || defined($cmd[3]));
Not new in this patch, but isn't this diagnostic misleading? It will
(falsely) claim too many arguments if $cmd[1] or $cmd[2] is an empty
string. Perhaps it should be reworded like the 'import' diagnostic and
say Invalid arguments for option.
We could even be more precise and separate the cases, i.e., die(Too
many arguments) when too many arguments are defined and die(Invalid
arguments) when there are empty strings.
Not sure if I should integrate it in this patch, though.
If you do choose to be more precise, it should be done as a separate
patch. Each conceptually distinct change should have its own patch.
Doing so makes changes easier to review and (generally) easier to
cherry-pick. For example, in this particular case, simplify
doubly-negated expressions is quite conceptually distinct from emit
more precise diagnostics. (Textually the changes may happen to
overlap, but conceptually they are unrelated.)
--
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 05/18] Turn double-negated expressions into simple expressions
Le 07/06/2013 22:25, Eric Sunshine a écrit : If you do choose to be more precise, it should be done as a separate patch. Each conceptually distinct change should have its own patch. Doing so makes changes easier to review and (generally) easier to cherry-pick. For example, in this particular case, simplify doubly-negated expressions is quite conceptually distinct from emit more precise diagnostics. (Textually the changes may happen to overlap, but conceptually they are unrelated.) OK, I will do another patch. -- 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
[PATCH 05/18] Turn double-negated expressions into simple expressions
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 |8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl
b/contrib/mw-to-git/git-remote-mediawiki.perl
index 68fd129..a6c7de2 100755
--- a/contrib/mw-to-git/git-remote-mediawiki.perl
+++ b/contrib/mw-to-git/git-remote-mediawiki.perl
@@ -136,16 +136,16 @@ while (STDIN) {
if (defined($cmd[0])) {
# Line not blank
if ($cmd[0] eq capabilities) {
- die(Too many arguments for capabilities\n) unless
(!defined($cmd[1]));
+ die(Too many arguments for capabilities\n) if
(defined($cmd[1]));
mw_capabilities();
} elsif ($cmd[0] eq list) {
- die(Too many arguments for list\n) unless
(!defined($cmd[2]));
+ die(Too many arguments for list\n) if
(defined($cmd[2]));
mw_list($cmd[1]);
} elsif ($cmd[0] eq import) {
- die(Invalid arguments for import\n) unless ($cmd[1]
ne !defined($cmd[2]));
+ die(Invalid arguments for import\n) if ($cmd[1] eq
|| defined($cmd[2]));
mw_import($cmd[1]);
} elsif ($cmd[0] eq option) {
- die(Too many arguments for option\n) unless ($cmd[1]
ne $cmd[2] ne !defined($cmd[3]));
+ die(Too many arguments for option\n) if ($cmd[1] eq
|| $cmd[2] eq || defined($cmd[3]));
mw_option($cmd[1],$cmd[2]);
} elsif ($cmd[0] eq push) {
mw_push($cmd[1]);
--
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
Re: [PATCH 05/18] Turn double-negated expressions into simple expressions
On Thu, Jun 6, 2013 at 3:34 PM, Célestin Matte
celestin.ma...@ensimag.fr wrote:
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 |8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl
b/contrib/mw-to-git/git-remote-mediawiki.perl
index 68fd129..a6c7de2 100755
--- a/contrib/mw-to-git/git-remote-mediawiki.perl
+++ b/contrib/mw-to-git/git-remote-mediawiki.perl
@@ -136,16 +136,16 @@ while (STDIN) {
if (defined($cmd[0])) {
# Line not blank
if ($cmd[0] eq capabilities) {
- die(Too many arguments for capabilities\n) unless
(!defined($cmd[1]));
+ die(Too many arguments for capabilities\n) if
(defined($cmd[1]));
mw_capabilities();
} elsif ($cmd[0] eq list) {
- die(Too many arguments for list\n) unless
(!defined($cmd[2]));
+ die(Too many arguments for list\n) if
(defined($cmd[2]));
mw_list($cmd[1]);
} elsif ($cmd[0] eq import) {
- die(Invalid arguments for import\n) unless ($cmd[1]
ne !defined($cmd[2]));
+ die(Invalid arguments for import\n) if ($cmd[1] eq
|| defined($cmd[2]));
mw_import($cmd[1]);
} elsif ($cmd[0] eq option) {
- die(Too many arguments for option\n) unless
($cmd[1] ne $cmd[2] ne !defined($cmd[3]));
+ die(Too many arguments for option\n) if ($cmd[1] eq
|| $cmd[2] eq || defined($cmd[3]));
Not new in this patch, but isn't this diagnostic misleading? It will
(falsely) claim too many arguments if $cmd[1] or $cmd[2] is an empty
string. Perhaps it should be reworded like the 'import' diagnostic and
say Invalid arguments for option.
mw_option($cmd[1],$cmd[2]);
} elsif ($cmd[0] eq push) {
mw_push($cmd[1]);
--
--
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
