Implement --color=auto|always|never, fix #2410 (#2552)

* Implement --color=auto|always|never, fix #2410

Also improves error message output for unsupported --color values.

* fix compile error with certain D versions

changelog/colors.dd

@@ -0,0 +1,6 @@
+The `--color` argument now accepts values `auto`, `never`, `always`
+
+The previous `automatic`, `on`, `off` values are still supported, but
+undocumented, because they are used in almost no other program like this. For
+consistency, with other Linux tools especially, we have implemented and switched
+the defaults to the widely-used `auto`, `never`, `always` values.

source/dub/commandline.d

@@ -168,7 +168,7 @@ struct CommandLineHandler
 			options.root_path = options.root_path.expandTilde.absolutePath.buildNormalizedPath;
 		}
 
-		final switch (options.color_mode) with (options.color)
+		final switch (options.colorMode) with (options.Color)
 		{
 			case automatic:
 				// Use default determined in internal.logging.initLogging().
@@ -524,11 +524,34 @@ struct CommonOptions {
 	bool help, annotate, bare;
 	string[] registry_urls;
 	string root_path;
-	enum color { automatic, on, off } // Lower case "color" in support of invalid option error formatting.
-	color color_mode = color.automatic;
+	enum Color { automatic, on, off }
+	Color colorMode = Color.automatic;
 	SkipPackageSuppliers skipRegistry = SkipPackageSuppliers.none;
 	PlacementLocation placementLocation = PlacementLocation.user;
 
+	deprecated("Use `Color` instead, the previous naming was a limitation of error message formatting")
+	alias color = Color;
+	deprecated("Use `colorMode` instead")
+	alias color_mode = colorMode;
+
+	private void parseColor(string option, string value) @safe
+	{
+		// `automatic`, `on`, `off` are there for backwards compatibility
+		// `auto`, `always`, `never` is being used for compatibility with most
+		// other development and linux tools, after evaluating what other tools
+		// are doing, to help users intuitively pick correct values.
+		// See https://github.com/dlang/dub/issues/2410 for discussion
+		if (!value.length || value == "auto" || value == "automatic")
+			colorMode = Color.automatic;
+		else if (value == "always" || value == "on")
+			colorMode = Color.on;
+		else if (value == "never" || value == "off")
+			colorMode = Color.off;
+		else
+			throw new ConvException("Unable to parse argument '--" ~ option ~ "=" ~ value
+				~ "', supported values: --color[=auto], --color=always, --color=never");
+	}
+
 	/// Parses all common options and stores the result in the struct instance.
 	void prepare(CommandArgs args)
 	{
@@ -553,12 +576,12 @@ struct CommonOptions {
 		args.getopt("q|quiet", &quiet, ["Only print warnings and errors"]);
 		args.getopt("verror", &verror, ["Only print errors"]);
 		args.getopt("vquiet", &vquiet, ["Print no messages"]);
-		args.getopt("color", &color_mode, [
+		args.getopt("color", &colorMode, &parseColor, [
 			"Configure colored output. Accepted values:",
-			"  automatic: Colored output on console/terminal,",
+			"       auto: Colored output on console/terminal,",
 			"             unless NO_COLOR is set and non-empty (default)",
-			"         on: Force colors enabled",
-			"        off: Force colors disabled"
+			"     always: Force colors enabled",
+			"      never: Force colors disabled"
 			]);
 		args.getopt("cache", &placementLocation, ["Puts any fetched packages in the specified location [local|system|user]."]);
 
@@ -629,6 +652,11 @@ class CommandArgs {
 	@property const(Arg)[] recognizedArgs() { return m_recognizedArgs; }
 
 	void getopt(T)(string names, T* var, string[] help_text = null, bool hidden=false)
+	{
+		getopt!T(names, var, null, help_text, hidden);
+	}
+
+	void getopt(T)(string names, T* var, void delegate(string, string) @safe parseValue, string[] help_text = null, bool hidden=false)
 	{
 		foreach (ref arg; m_recognizedArgs)
 			if (names == arg.names) {
@@ -642,7 +670,10 @@ class CommandArgs {
 		arg.names = names;
 		arg.helpText = help_text;
 		arg.hidden = hidden;
-		m_args.getopt(config.passThrough, names, var);
+		if (parseValue is null)
+			m_args.getopt(config.passThrough, names, var);
+		else
+			m_args.getopt(config.passThrough, names, parseValue);
 		arg.value = *var;
 		m_recognizedArgs ~= arg;
 	}

test/colored-output.sh

@@ -4,25 +4,32 @@
 
 cd ${CURR_DIR}/1-exec-simple
 
-# Test that --color=off disables colors correctly
-${DUB} build --color=off --compiler=${DC} 2>&1 | { ! \grep $'^\x1b\[' -c; }
+# Test that --color=never disables colors correctly
+printf "Expecting 0: "
+${DUB} build --color=never --compiler=${DC} 2>&1 | { ! \grep $'^\x1b\[' -c; }
 
-# Test that --color=automatic detects no TTY correctly
-${DUB} build --color=automatic --compiler=${DC} 2>&1 | { ! \grep $'^\x1b\[' -c; }
+# Test that --color=auto detects no TTY correctly
+printf "Expecting 0: "
+${DUB} build --color=auto --compiler=${DC} 2>&1 | { ! \grep $'^\x1b\[' -c; }
 
-# Test that no --color= has same behaviour as --color=automatic
+# Test that no --color= has same behaviour as --color=auto
+printf "Expecting 0: "
 ${DUB} build --compiler=${DC} 2>&1 | { ! \grep $'^\x1b\[' -c; }
 
-# Test that --color=on enables colors in any case
-${DUB} build --color=on --compiler=${DC} 2>&1 | \grep $'^\x1b\[' -c
+# Test that --color=always enables colors in any case
+printf "Expecting non-0: "
+${DUB} build --color=always --compiler=${DC} 2>&1 | \grep $'^\x1b\[' -c
 
 # Test forwarding to dmd flag -color
 
-# Test that --color=on set dmd flag -color
-${DUB} build -v --color=on --compiler=${DC} -f 2>&1 | \grep '\-color' -c
+# Test that --color=always set dmd flag -color
+printf "Expecting non-0: "
+${DUB} build -v --color=always --compiler=${DC} -f 2>&1 | \grep '\-color' -c
 
-# Test that --color=off set no dmd flag
-${DUB} build -v --color=off --compiler=${DC} -f 2>&1 | { ! \grep '\-color' -c; }
+# Test that --color=never set no dmd flag
+printf "Expecting 0: "
+${DUB} build -v --color=never --compiler=${DC} -f 2>&1 | { ! \grep '\-color' -c; }
 
-# Test that --color=automatic set no dmd flag
-${DUB} build -v --color=automatic --compiler=${DC} -f 2>&1 | { ! \grep '\-color' -c; }
+# Test that --color=auto set no dmd flag
+printf "Expecting 0: "
+${DUB} build -v --color=auto --compiler=${DC} -f 2>&1 | { ! \grep '\-color' -c; }