-
-
Notifications
You must be signed in to change notification settings - Fork 209
feat: add failure_message
option for tasks (#2080)
#2081
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.
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
base: master
Are you sure you want to change the base?
feat: add failure_message
option for tasks (#2080)
#2081
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
5fe14bd
to
592427a
Compare
I rebased this branch on tsvikas/feat/task-error branch and updated it to work with that change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you rebase onto master
please? #1991 has been merged.
* 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 copier-org#2080: copier-org#2080
592427a
to
498a5f5
Compare
@sisp I have rebased on master. Let me know if any other adjustments are needed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
copier/_template.py
Outdated
@@ -169,12 +169,17 @@ class Task: | |||
working_directory: | |||
The directory from inside where to execute the task. | |||
If `None`, the project directory will be used. | |||
|
|||
failure_message: | |||
Provides a message to pritn if the task fails. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Provides a message to pritn if the task fails. | |
Provides a message to print if the task fails. |
Still there 🤷 🤓
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finally fixed in d489cde!
copier/errors.py
Outdated
if not message: | ||
message = subprocess.CalledProcessError.__str__(self) | ||
UserMessageError.__init__(self, message=message) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd keep the default error message as before and only replace it by the custom message if provided:
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was adjusted in d489cde. Please check out the change (adding index to TaskError) and let me know what you think!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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__
.
copier/_main.py
Outdated
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}.", | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When task.failure_message
is None
, then I'd simply fall back to the default error message defined in TaskError
:
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was adjusted in d489cde. Please check out the change (adding index to TaskError) and let me know what you think!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
- Add an `index` attribute to TaskError - TaskError adds its own prefix: "Task <i> failed: <message>" - Fixed a typo - Updated unit tests
failure_message
option is provided, show that message: "Task N failed: a nice failure message"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