Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Permit Mojo::Util::tablify to work on non-rectangular arrays. #1132

Closed
wants to merge 6 commits into from

Conversation

lindleyw
Copy link
Contributor

Summary

Should a non-rectangular array (one in which not all rows have equal numbers of columns) be passed to Mojo::Util::tablify, the resulting columns or entirely empty rows will be skipped rather than producing errors from missing sprintf() arguments.

Motivation

Users who write simple classes based on Mojolicious::Command should be able to produce arrays for table output, perhaps from a variety of functions or a JSON structure, without having to ensure that each row has an equal number of columns.

References

https://irclog.perlgeek.de/mojo/2017-09-14#i_15164351

@jberger
Copy link
Member

jberger commented Sep 14, 2017

Well clearly you need to make sure it passes existing tests. Once you've done that can you please add a test and verify the formatting; that second line looks longer than the character limit.

lib/Mojo/Util.pm Outdated
@@ -282,8 +282,8 @@ sub tablify {
}
}

my $format = join ' ', map({"\%-${_}s"} @spec[0 .. $#spec - 1]), '%s';
return join '', map { sprintf "$format\n", @$_ } @$rows;
my @formats = (map({"\%-${_}s"} @spec[0 .. $#spec - 1]),'%s');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parentheses around the map are unnecessary. This should be run through perltidy.

lib/Mojo/Util.pm Outdated
my $format = join ' ', map({"\%-${_}s"} @spec[0 .. $#spec - 1]), '%s';
return join '', map { sprintf "$format\n", @$_ } @$rows;
my @formats = (map({"\%-${_}s"} @spec[0 .. $#spec - 1]),'%s');
return join '', map { sprintf join(' ', @formats[0..scalar @$_ - 1]) . "\n", @$_ } @$rows;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Columns should be joined on 2 spaces, this appears to be the cause of test failures. @formats[0..scalar @$_ - 1] can be shortened to @formats[0..$#$_]

@lindleyw
Copy link
Contributor Author

I am still unsure why: $ TEST_UNIX=1 prove t/mojo never reports these failures on my system locally.

@jhthorsen
Copy link
Member

jhthorsen commented Sep 15, 2017

I'm neutral to the change, though I can't come up with a case myself where I would give tablify() "non-rectangular arrays".

(Tests are still missing)

@jberger
Copy link
Member

jberger commented Sep 15, 2017

I lean slightly with @jhthorsen, especially since the data could be rectangularlized fairly easily by the user (push to unfilled rows with ''). This is compounded by because this is positional (rather than say named) the empty columns must come at the end (empty columns in the middle will have to be rectangularized manually in order to make sense).

That said, if it can be made more flexible and DWIM without any cost, I don't see any reason to oppose (once the PR works and tests are added).

@jhthorsen
Copy link
Member

@lindleyw: Could you please provide a real life example for when you don't have rectangular data?

@latk
Copy link

latk commented Sep 18, 2017

@lindleyw Running prove t/mojo will use the installed version of Mojo. Use the -lflag to add the lib directory to the test script's @INC. In general, prove is invoked as prove -lr t. See the prove docs for more details.

Since this is a MakeMaker based dist, you can also run the tests as perl Makefile.PL && make test.

@kraih
Copy link
Member

kraih commented Oct 25, 2017

No need to add extra lines.

diff --git a/lib/Mojo/Util.pm b/lib/Mojo/Util.pm
index c6c2e3491..5438d1621 100644
--- a/lib/Mojo/Util.pm
+++ b/lib/Mojo/Util.pm
@@ -282,8 +282,8 @@ sub tablify {
     }
   }

-  my $format = join '  ', map({"\%-${_}s"} @spec[0 .. $#spec - 1]), '%s';
-  return join '', map { sprintf "$format\n", @$_ } @$rows;
+  my @fs = (map({"\%-${_}s"} @spec[0 .. $#spec - 1]), '%s');
+  return join '', map { sprintf join('  ', @fs[0 .. $#$_]) . "\n", @$_ } @$rows;
 }

 sub term_escape {

@kraih
Copy link
Member

kraih commented Nov 2, 2017

Has this pull request been abandoned?

@lindleyw
Copy link
Contributor Author

lindleyw commented Nov 2, 2017

I was away for awhile. Back now; will proceed.

@kraih
Copy link
Member

kraih commented Nov 5, 2017

I kinda wanted to merge this for the last release, but the pull request has still not been fixed. ☹️

@kraih
Copy link
Member

kraih commented Nov 6, 2017

Resolved by #1150.

@kraih kraih closed this Nov 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants