Re: [PATCH v5 07/10] send-email: reduce dependancies impact on parse_address_line
From: Matthieu Moy <hidden>
Date: 2016-06-15 23:05:25
This is the last message I received in the series, and it's labeled 07/10. Is that normal?
parse_address_line had not the same behavior whether the user had
had not -> did not have
I've added the function in Git.pm as suggested. I've also added a test named t9000-addresses.sh (I've read the README to name tests but I'm not sure about the name of this test). I made a separated test (t9000-addresses.sh) because I think it's better not to pollute t9001-send-email with this.
Sounds good to me.
About the test itself, file t/t9000-addresses.sh is just a copy/paste of t/t0202-gettext-perl.sh. For the perl part, the TODO tests are verbose: they print out commands whereas test_expect_success doesn't.
It seems it's how Test::More works. I'd keep it like this, but I have no real experience with Test::More.
We can redirect todo_output to a variable but I've not found better... (Maybe someone has the solution here ?). Also there's no summary at the end of the test (as with other perl tests).
You can get the 1..44 at the end with printf "1..%d\n", Test::More->builder->current_test; This is what t9700/test.pl does.
quoted hunk ↗ jump to hunk
diff --git a/perl/Git.pm b/perl/Git.pm index 9026a7b..97633e9 100644 --- a/perl/Git.pm +++ b/perl/Git.pm@@ -1584,6 +1584,73 @@ sub DESTROY { $self->_close_cat_blob(); } +=item parse_mailboxes + +Returns an array of mailboxes extracted from a string.
Imperative tone => Return, not Returns. I would have put parse_mailbox near ident_person because both functions are somehow about email.
+BEGIN { use_ok('Git') }
+BEGIN { use_ok('Mail::Address') }This will fail if Mail::Address is not available. It would be better to declare Mail::Address as a prerequisite in t9000-address.sh (like what you're already doing for Test::More). Good job, modulo these minor details, the series looks good to me. -- Matthieu Moy http://www-verimag.imag.fr/~moy/