Skip to content

Update Formatter\show_table to use Runner->in_color rather than shouldColorize #5804

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.

Sign up for GitHub

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

Merged
schlessera merged 1 commit into from
Feb 5, 2024
Merged

Update Formatter\show_table to use Runner->in_color rather than shouldColorize #5804

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion php/WP_CLI/Formatter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,7 @@ private function show_multiple_fields( $data, $format, $ascii_pre_colorized = fa
private static function show_table( $items, $fields, $ascii_pre_colorized = false ) {
$table = new Table();

$enabled = Colors::shouldColorize();
$enabled = WP_CLI::get_runner()->in_color();
Copy link
Copy Markdown
Member

@danielbachhuber danielbachhuber Nov 4, 2023

Choose a reason for hiding this comment

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

I don't love adding a dependency on WP_CLI::get_runner() inside of this Formatter class.

My first thought was to add a new class-level $in_color variable that can be passed when instantiated:

diff --git a/php/WP_CLI/Formatter.php b/php/WP_CLI/Formatter.php
index 8dd354f5..74fd714c 100644
--- a/php/WP_CLI/Formatter.php
+++ b/php/WP_CLI/Formatter.php
@@ -33,7 +33,7 @@ class Formatter {
 	 * @param string|bool $prefix Check if fields have a standard prefix.
 	 * False indicates empty prefix.
 	 */
-	public function __construct( &$assoc_args, $fields = null, $prefix = false ) {
+	public function __construct( &$assoc_args, $fields = null, $prefix = false, $in_color = true ) {
 		$format_args = [
 			'format' => 'table',
 			'fields' => $fields,
@@ -53,8 +53,9 @@ class Formatter {
 
 		$format_args['fields'] = array_map( 'trim', $format_args['fields'] );
 
-		$this->args   = $format_args;
-		$this->prefix = $prefix;
+		$this->args     = $format_args;
+		$this->prefix   = $prefix;
+		$this->in_color = $in_color
 	}
 
 	/**

However, this would just put the odd dependency elsewhere:

wp-cli/php/utils.php

Lines 362 to 369 in 9239cff

function format_items( $format, $items, $fields ) {
$assoc_args = [
'format' => $format,
'fields' => $fields,
];
$formatter = new Formatter( $assoc_args );
$formatter->display_items( $items );
}

Because Formatter already calls WP_CLI::print_value(), Utils\write_csv(), etc., I think it's fine to call out to WP_CLI:: or a utility function. We could add an in_color() method like this:

diff --git a/php/class-wp-cli.php b/php/class-wp-cli.php
index c775bbc3..67d29c7a 100644
--- a/php/class-wp-cli.php
+++ b/php/class-wp-cli.php
@@ -728,6 +728,20 @@ class WP_CLI {
 		unset( self::$deferred_additions[ $name ] );
 	}
 
+	/**
+	 * Whether or not WP-CLI is currently in color.
+	 *
+	 * @param bool|null $set Whether or not to set the status.
+	 * @return bool
+	 */
+	public static function in_color( $set = null ) {
+		static $in_color;
+		if ( ! is_null( $set ) ) {
+			$in_color = (bool) $set;
+		}
+		return $in_color;
+	}
+
 	/**
 	 * Display informational message without prefix, and ignore `--quiet`.
 	 *

We'd then use it like this:

diff --git a/php/WP_CLI/Formatter.php b/php/WP_CLI/Formatter.php
index 8dd354f5..7f2500f5 100644
--- a/php/WP_CLI/Formatter.php
+++ b/php/WP_CLI/Formatter.php
@@ -301,7 +301,7 @@ class Formatter {
 	private static function show_table( $items, $fields, $ascii_pre_colorized = false ) {
 		$table = new Table();
 
-		$enabled = Colors::shouldColorize();
+		$enabled = WP_CLI::in_color();
 		if ( $enabled ) {
 			Colors::disable( true );
 		}
diff --git a/php/WP_CLI/Runner.php b/php/WP_CLI/Runner.php
index 9e7ceb27..f4d7948c 100644
--- a/php/WP_CLI/Runner.php
+++ b/php/WP_CLI/Runner.php
@@ -938,6 +938,7 @@ class Runner {
 		} else {
 			$this->colorize = $this->config['color'];
 		}
+		WP_CLI::in_color( $this->colorize );
 	}
 
 	public function init_logger() {

This is all somewhat spaghetti logic, but I think what I propose is marginally better than calling WP_CLI::get_runner() directly.

Thoughts?

Copy link
Copy Markdown
Member

@schlessera schlessera Feb 5, 2024

Choose a reason for hiding this comment

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

@danielbachhuber I agree that this dependency is not great. However, this has been one of the longstanding architectural issues that cannot easily be redressed, and references to the WP_CLI::get_runner() are distributed all across the codebase. Therefore, I think it doesn't make sense to add additional spaghetti for this one instance when we cannot at the same time fix the bigger picture. I'd opt for keeping with the WP_CLI::get_runner() for now until we decide for a bigger rewrite.

Copy link
Copy Markdown
Member

@danielbachhuber danielbachhuber Feb 5, 2024

Choose a reason for hiding this comment

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

@schlessera Ok, sounds good.

if ( $enabled ) {
Colors::disable( true );
}
Expand Down