Should gracefully fail when an invalid colour formatter is specified

[colinbendell]

When an invalid colour is used, the formatter raises an exception and interrupts the workflow. Instead, this should be gracefully handled so the workflow is not interrupted. At most, a warning should be emitted to signal the invalid formatting.

For example:

CLI::UI.fmt("{{grey:this is a grey (gray?) message}}")

Will cause this stack trace:

cli/ui/formatter.rb:113:in `fetch': key not found: "gray" (KeyError)
	 9: from cli-ui/lib/cli/ui.rb:114:in `fmt'
	 8: from cli/ui/formatter.rb:88:in `format'
	 7: from cli/ui/formatter.rb:88:in `each_with_object'
	 6: from cli/ui/formatter.rb:88:in `each'
	 5: from cli/ui/formatter.rb90:in `block in format'
	 4: from cli/ui/formatter.rb:110:in `apply_format'
	 3: from cli/ui/formatter.rb:110:in `each_with_object'
	 2: from cli/ui/formatter.rb:110:in `each'
	 1: from cli/ui/formatter.rb:112:in `block in apply_format'

[lugray]

I'd be happy with raising a better exception, which could then be handled however you choose, but passing a non-supported format code is definitely an error and should be handled that way, so that it can be addressed properly.

[colinbendell]

I disagree here. Because it's often used in open source tools, any error that is shipped with formatting errors interrupts the user from completing a task. The task is interrupted and the user does not have any control or remediation. Users should only see stack traces and errors when a) there is an error state in their environment (which they need to remediate) or b) there is a bug in the tool which, if it were to proceed would have unexpected and potentially disastrous results. In both cases, an error is appropriate because the tool is not able to execute the desired business logic and outcome.

Formatted output, (and I'd argue other functionality like logging) are not critical errors that should interrupt the human. It is a bug, certainly, but it is a non critical bug. Interrupting the human on this 'technicality' is very disruptive and the human has no recourse except to wait for a bugfix. If the action the human is trying to accomplish is mission critical for them, then we have interrupted the workflow for no consequential reason.

It is like having your prescription rejected by the pharmacist because the doctor had a spelling mistake in the pharmacy address line. Embarrassing yes, but very inconvenient to the patient.

[colinbendell]

@lugray, A very real case and point is Shopify/shopify-cli#1388 where the developer was prevented from pushing a theme, interrupting their development flow because of a text formatting error. This cost the developer hours and it took 2weeks to repair. Treating formatting as errors, not warnings, has cost several developer-entrepreneur hundreds if not thousands of productivity dollars.

[colinbendell]

Yes, that's why I preserved the raised exception when TEST environment variable is set. Happy to extend that to another context that captures development and rake test environments. ENV['TEST'] was just a convenient environment variable that is commonly set but could use any other convention.

That covers running cli-ui tests, but we have no idea what conventions are used in consumers' test/dev environments. I'd be happy to accept a pr to incorporate support for CLI::UI::Formatter.ignore_invalid_codes = true. This would default to false, but consumers could choose to set it to true.

There are two arguments now:

1. Whether formatting is a reason to interrupt CLI business logic
2. Whether ENV['TEST'] is sufficient enough to infer that the library is being used in dev or test mode by the calling CLI tool.

For {1}, I strongly maintain that the library should be generous - particularly when it is being invoked by a user. In this case it is not the developer that has integrated this library, but the end user who has a job to do. Interrupting business logic because of errors in the business logic is appropriate to avoid error states and corruption. Interrupting business logic because of cosmetic errors is not appropriate for the tool.

Further, we need to be generous to the developer who is integrating this library. They don't know what they don't know. They don't know that they could be bitten (like we were) by selecting an invalid colour. Requiring that the developer have foreknowledge to capture such states is a level of cognitive load that I don't want to push on to that developer. This is why, if anything I would advocate for ignore_invalid_codes = true be the default and the user would have to opt-out of generous mode and opt-into strict mode.

As for {2}, we do know that the developer likely is implementing TESTs. The TESTenvironment variable is commonly exposed and would be a good way to automatically trigger strict-mode. This way a CLI would error on formatting errors in their specs, but when the CLI is launched to production, this underlying library would return to generous mode.

[lugray]

I gave the puts "a" + 2 example because this could equally be seen as presentational logic. The format codes are a kind of DSL that we use within CLI::UI. If there's an error, it's an error. How you handle that in your code is up to you.

[lugray]

Closing as stale.