search

shlomif / Text-Table

CPAN Distribution to render text / ASCII-art / Unicode tables
https://metacpan.org/release/Text-Table
ISC License
4 stars 6 forks source link

Close shlomif/Text-Table#3 #4

Closed zbentley closed 8 years ago

zbentley commented 8 years ago

Existing tests/build all pass. New tests fail for me without module changes.

shlomif commented 8 years ago

Hi @zbentley !

Thanks for the issue report and this pull-request. That put aside - I have some comments:

  1. If you're going to hard code the boolean overloading of the instance to 1/True, then you should also remove the test for conditionality of it where it mattered in lib/Text/Table.pm , e.g:
if ($tb) {
   CODE
}

to:

CODE
  1. The new test file is lacking Test::Count annotations, see:

http://www.shlomifish.org/open-source/resources/how-to-contribute-to-my-projects/HACKING.html

  1. I prefer to make use of "scalar(...)"/etc. inside Test::More's "ok(...)" to avoid a possibility of an empty list as false in list context.

Please rework all those.

zbentley commented 8 years ago
  1. I don't see any code in Text/Table.pm that tests if ( $tb ) other than the test I added, which exists to verify the boolean overloading truthiness behavior. If such code exists, please let me know where it is and I will fix it. If the desired behavior is for truthiness to change based on presence of data, I can make that change as well. However, I think that truthiness based on table contents is a bit less obvious to users: should a table be "false" if column headings have been configured, but no rows added? That might be useful, but would be a behavior change and would need to be documented; I think the simpler change runs less risk of breaking existing code.
  2. Adding annotations and manifest entries in PR momentarily.
  3. That is a good practice. Since delete on (non-tied or overloaded) hashes will always return a scalar or undef, it is not necessary here. If the value being tested on were a more complex expression, using scalar would be in order. See: perl -Mstrict -MData::Dumper -e 'my %a; my $b = delete $a{foo}; my @b = delete $a{foo}; print Dumper($b); print Dumper(\@b); print scalar(@b);'
shlomif commented 8 years ago

@zbentley : hi! Thanks for returning to me. Regarding No. 1 - the boolean check - apparently I misread your original bug report, and didn't notice you performed that check for truthness in the code yourself. Thanks for the other fixes - I'll review your pull-request and hopefully commit it and release a new version.

zbentley commented 8 years ago

Thanks for taking the time to review it!

Be aware that this is technically a small behavior change: tables that previously stringified to the empty string and returned falsy in boolean context will now return true. I doubt that there are many use cases that not only perform boolean checks on tables, but also do important things in the negative case, but there are probably some.