feat: add failure_message option for tasks (#2080)

[dusktreader]

• Do not show stack trace on task failure
• If a failure_message option is provided, show that message: "Task N failed: a nice failure message"
• If no failure_message option is provided, show the subprocess exception: "Task N failed: Command 'might-fail' returned non-zero exit status 1."

Addresses Issue #2080: #2080

[sisp]

Thanks for contributing this feature, @dusktreader! 🙏 I think it's a good idea, especially for simple task commands with only one possible non-zero exit status and a clear meaning of the cause. I have a few minor remarks and a concern regarding backwards compatibility – see inline comments.

[dusktreader]

I rebased this branch on tsvikas/feat/task-error branch and updated it to work with that change.

[sisp]

Could you rebase onto master please? #1991 has been merged.

[dusktreader]

@sisp I have rebased on master. Let me know if any other adjustments are needed.

[sisp]

I have a few minor remarks. And a general nit-picky one: Prepending Task <n> failed: <message> also to the default error message is technically not related to adding a failure_message option for tasks. In fact, I think this prefix makes sense for custom failure messages, as we don't include the task command there, so it helps identify the erroneous task, while the default error message does include the command, so the task index doesn't seem necessary. Please remember to update expected messages in the tests accordingly.

[sisp]

Suggested change

	Provides a message to pritn if the task fails.
	Provides a message to print if the task fails.

Still there 🤷 🤓

[dusktreader]

Finally fixed in d489cde!

[sisp]

I'd keep the default error message as before and only replace it by the custom message if provided:

Suggested change

	if not message:
	message = subprocess.CalledProcessError.__str__(self)
	UserMessageError.__init__(self, message=message)
	if not message:
	message = f"Task {command!r} returned non-zero exit status {returncode}."
	UserMessageError.__init__(self, message)

[dusktreader]

I did that because the message from subprocess.CalledProcessError.__str__ is almost identical:

Command '<whatever>' returned non-zero exit status <code>.

vs

Task '<whatever>' returned non-zero exit status <code>.

But, I put the default message back how it was.

[dusktreader]

This was adjusted in d489cde. Please check out the change (adding index to TaskError) and let me know what you think!

[sisp]

I see. I just fully realized that the task index is also an explicit addition in this PR. TBH, I'm not fully sold to the task index information. Intuitively, I'd say that this PR adds a custom task failure message that replaces the automatic one when provided – that's it. This means two message variants:

• Task '<whatever>' returned non-zero exit status <code>.
• Task '<whatever>' failed: <failure_message>.

Then, the PR is scoped to adding the custom failure message. WDYT?

I did that because the message from subprocess.CalledProcessError.__str__ is almost identical: [...]

Ah, right, now this makes sense again and I'd say we should keep the custom message instead of subprocess.CalledProcessError.__str__.

[sisp]

When task.failure_message is None, then I'd simply fall back to the default error message defined in TaskError:

Suggested change

	if task.failure_message:
	message = self._render_string(task.failure_message, extra_context)
	else:
	message = f"{task_cmd!r} returned non-zero exit status {process.returncode}"
	raise TaskError.from_process(
	process,
	message=f"Task {i + 1} failed: {message}.",
	)
	if task.failure_message:
	message = f"Task {i + 1} failed: {self._render_string(task.failure_message, extra_context)}"
	else:
	message = None
	raise TaskError.from_process(process, message)

[dusktreader]

The issue is that you lose the context of which task failed because the message is no longer prefixed with "Task <i + 1>".

What do you think of adding an index attribute to TaskError so that it can add the prefix itself?

[dusktreader]

This was adjusted in d489cde. Please check out the change (adding index to TaskError) and let me know what you think!

[sisp]

TBH, I'm not fully sold to the task index information because a user would need to check the template's copier.yml to know the command that failed. IMO, ideally a user should not need to check the template implementation upon error. See also my other related comment.