Skip to content
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

FIX: Use standard error for die(), dieWithHelp(), and error messages #404

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

inkarkat
Copy link
Member

Before submitting a pull request, please make sure the following is done:

  • Fork the repository and create your branch from master.
  • If you've added code that should be tested, add tests!
  • Ensure the test suite passes.
  • Format your code with ShellCheck.
  • Include a human-readable description of what the pull request is trying to accomplish.
  • Steps for the reviewer(s) on how they can manually QA the changes.
  • Have a fixes #XX reference to the issue that this pull request fixes.

By convention, error output should be printed to standard error, not standard out. Same for the usage help that may accompany the error message.
To indicate that something went wrong (e.g. the task already was unprioritized).
Note: For actions that handle multiple ITEMs in a loop, we cannot use die() as that would abort processing of any following ITEM(s). Instead, use a status variable and set it to 1 on error, then exit at the end.
@@ -1211,9 +1212,11 @@ case $action in
echo "TODO: $item deprioritized."
fi
else
echo "TODO: $item is not prioritized."
echo >&2 "TODO: $item is not prioritized."
status=1
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it necessary here to leave the whole process with failure exit status? Maybe you could test if other $items succeed?

Copy link
Member Author

Choose a reason for hiding this comment

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

For me, failure is the inability to do what I was told, even if I partially succeeded. The user still has the action's output to see which items succeeded and which didn't; the exit status is more of interest from a scripting perspective; the shell doesn't visualize the exit status by default (but I color the following prompt red if there was an error). And from that scripting perspective, it's more interesting whether the whole command succeeded, not whether it got partially done. (And you can always use a loop and invoke the command with each item separately to obtain that information.)

@chrysle
Copy link
Contributor

chrysle commented Jan 22, 2023

Could you help me out how to use this PR as a base for mine? I can't find any description concerning this.

@inkarkat
Copy link
Member Author

@chrysle There's still a long queue of PRs waiting to be merged, so I don't think it's worth the effort to properly base your changes on mine. Just add the >&2 to the die() function manually, and the completion test should pass again (but other tests that I've adapted in this PR will fail). As long as this PR gets merged before your, everything will be fine ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants