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

print a nicer error on timeout/cancel #137

Merged
merged 1 commit into from
Dec 13, 2018
Merged

print a nicer error on timeout/cancel #137

merged 1 commit into from
Dec 13, 2018

Conversation

Stebalien
Copy link
Member

Don't say "context canceled". Just say "canceled".

fixes ipfs/kubo#3785

*Don't* say "context canceled". Just say "canceled".

fixes ipfs/kubo#3785
@Stebalien Stebalien requested a review from keks as a code owner December 7, 2018 18:29
@ghost ghost assigned Stebalien Dec 7, 2018
@ghost ghost added the status/in-progress In progress label Dec 7, 2018
case context.DeadlineExceeded:
msg = "timed out"
default:
msg = err.Error()
}
Copy link
Member Author

Choose a reason for hiding this comment

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

We don't need a cmdkit.Error, we just need the error message.

@Stebalien
Copy link
Member Author

This now prints:

Error: canceled

When a user hits ^C and:

Error: timed out

When the command times out.

@djdv
Copy link
Contributor

djdv commented Dec 9, 2018

I like this change either way, but wonder if prepending "request" is necessary.

Error: request canceled
Error: request timed-out

vs

Error: canceled
Error: timed-out

Edit: I feel like the verbose line information etc. was the problem in the original issue rather than an additional word "context".
Or perhaps, cancellations shouldn't be considered an error to the canceler.
Need @whyrusleeping's insight 👁

@Stebalien
Copy link
Member Author

Edit: I feel like the verbose line information etc. was the problem in the original issue rather than an additional word "context".

That was probably the motivation for the original issue but I've also seen users confused about the term "context". That's a go-ism and, really, an implementation detail.

At the end of the day, we just want to communicate:

  1. That the command failed.
  2. Why it failed.

I'd also like to avoid "request" because that really doesn't have anything to do with what the user is trying to do (they shouldn't even need to consider the fact that ipfs ... is sending a "request" to the daemon).

@Stebalien
Copy link
Member Author

We can bikeshed this in a followup PR.

@Stebalien Stebalien merged commit 18fbb38 into master Dec 13, 2018
@ghost ghost removed the status/in-progress In progress label Dec 13, 2018
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.

Fix ugly output when killing a command
3 participants