Add test for line breaks in table view#6055
Add test for line breaks in table view#6055mrsdizzie merged 11 commits intowp-cli:mainwp-cli/wp-cli:mainfrom BhargavBhandari90:line-break-improvementBhargavBhandari90/wp-cli:line-break-improvementCopy head branch name to clipboard
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Thanks for your contribution, this looks like a great start! Yes, the fields need to be handled dynamically and not hardcoded, but then it should work nicely. So basically whenever there's one or more fields with a newline, all other fields without one need a blank line added. Tests could be added to |
|
Thank you @swissspidy for your feedback. I'll improve the code. Tests could be added to features/formatter.feature
|
|
Hi @swissspidy I've tried to improve code and added a test. Aslo, I think this line break thing currently we should limit to Let me know what you think. |
|
Thanks for adding the test, that's very useful for further iterations. The 3 remaining failing ones are currently known and can be ignored. I understand your concern, but the formatter code needs to be command-agnostic and should not have any hard coded field references. I think that should be possible. Let me know if you'd like any help in making that last step happen. |
|
Wouldn't this need to be fixed in the function that generates table rows so that it does work for different types of output: (I attempted a quick fix, but I wasn't familiar enough with the code to get it to work 100% when the table wraps because of a size constraint). I think the entire logic would need to be reworked since right now it attempts to remove newlines rather than understand them properly as additional rows. |
|
OK actually I think this might do it: diff --git a/lib/cli/table/Ascii.php b/lib/cli/table/Ascii.php
index ff5c3d9..b2505e6 100644
--- a/lib/cli/table/Ascii.php
+++ b/lib/cli/table/Ascii.php
@@ -136,31 +136,32 @@ class Ascii extends Renderer {
if ( count( $row ) > 0 ) {
$extra_rows = array_fill( 0, count( $row ), array() );
- foreach( $row as $col => $value ) {
- $value = $value ?: '';
- $value = str_replace( array( "\r\n", "\n" ), ' ', $value );
-
- $col_width = $this->_widths[ $col ];
- $encoding = function_exists( 'mb_detect_encoding' ) ? mb_detect_encoding( $value, null, true /*strict*/ ) : false;
+ foreach ( $row as $col => $value ) {
+ $value = $value ?: '';
+ $col_width = $this->_widths[ $col ];
+ $encoding = function_exists( 'mb_detect_encoding' ) ? mb_detect_encoding( $value, null, true /*strict*/ ) : false;
$original_val_width = Colors::width( $value, self::isPreColorized( $col ), $encoding );
- if ( $col_width && $original_val_width > $col_width ) {
- $row[ $col ] = \cli\safe_substr( $value, 0, $col_width, true /*is_width*/, $encoding );
- $value = \cli\safe_substr( $value, \cli\safe_strlen( $row[ $col ], $encoding ), null /*length*/, false /*is_width*/, $encoding );
- $i = 0;
- do {
- $extra_value = \cli\safe_substr( $value, 0, $col_width, true /*is_width*/, $encoding );
- $val_width = Colors::width( $extra_value, self::isPreColorized( $col ), $encoding );
- if ( $val_width ) {
- $extra_rows[ $col ][] = $extra_value;
- $value = \cli\safe_substr( $value, \cli\safe_strlen( $extra_value, $encoding ), null /*length*/, false /*is_width*/, $encoding );
- $i++;
- if ( $i > $extra_row_count ) {
- $extra_row_count = $i;
+ if ( $col_width && ( $original_val_width > $col_width || strpos( $value, "\n" ) !== false ) ) {
+ $split_lines = preg_split( '/\r\n|\n/', $value );
+
+ $wrapped_lines = [];
+ foreach ( $split_lines as $line ) {
+ do {
+ $wrapped_value = \cli\safe_substr( $line, 0, $col_width, true /*is_width*/, $encoding );
+ $val_width = Colors::width( $wrapped_value, self::isPreColorized( $col ), $encoding );
+ if ( $val_width ) {
+ $wrapped_lines[] = $wrapped_value;
+ $line = \cli\safe_substr( $line, \cli\safe_strlen( $wrapped_value, $encoding ), null /*length*/, false /*is_width*/, $encoding );
}
- }
- } while( $value );
- }
+ } while ( $line );
+ }
+ $row[ $col ] = array_shift( $wrapped_lines );
+ foreach ( $wrapped_lines as $wrapped_line ) {
+ $extra_rows[ $col ][] = $wrapped_line;
+ ++$extra_row_count;
+ }
+ }
}
}
What do you think about that? the row function already had logic for appending additional rows but it was only used for wrapping based on width. Now it takes newlines into account as well (phpunit tests still pass too) |
|
Wow @mrsdizzie This looks great. Lets see what @swissspidy say about this. |
|
Let‘s ship it! Thanks @mrsdizzie! |
|
OK I've created wp-cli/php-cli-tools#179 for this. Maybe we can keep this PR open for now and still merge the new test afterwards, since that seems useful and there aren't behat tests for php-cli-tools |
|
Hmm now the output in the test is like this: @mrsdizzie what am I missing? |
|
This must have something to do with there being no tty and it behaving differently -- you can see similar behavior locally: Maybe there is a different code path for no tty that the earlier changes don't run on? Or maybe it is collapsing empty columns when no tty? will have to investigate that 🕵️♀️ Edit: All it does is turn a row array into a string spaced by tabs. I guess that one needs to be changed as well. Or alternatively we can use |
mrsdizzie
left a comment
There was a problem hiding this comment.
Thanks for pushing to get this fixed and including the test
Edit: This ended up being fixed in wp-cli/php-cli-tools#182 so now this PR just includes a behat test to verify that change works
Fixes: wp-cli/entity-command#262
Before:
After I modified
Formatter.phpThis is I am marking as WIP as I need some help to improve this.
I've addes some static keys (
post_id317 ,meta_key318 ,meta_value319 ) for the array manipulation in the loop.This should be coming from
$fieldsI guess.