search

go-ping / ping

ICMP Ping library for Go
MIT License
1.3k stars 343 forks source link

Library should return errors and not printf messages #16

Closed Springuin closed 1 year ago

Springuin commented 6 years ago

It's probably just my opinion, but I think having Printf's when an error occurs is not the best option. There is no easy way to divert the error messages to something else, so you practically have no choice but to have the error messages end up at stdout. May I suggest adding a feature to divert the output (to a different stream, to a logger, ....) or adding a function that disables the messages and just return an error?

CHTJonas commented 3 years ago

Thanks for raising! We're aware of this and agree that it needs fixing; we're looking into how best to go about this.

stevenaldinger commented 3 years ago

the PR to close this issue looks like it's covering the logging case but not the error case. I think that's an awesome start but wondering if you were saying you agree that it needs to be passing back errors too or just that there needs to be a way to control log output?

CHTJonas commented 3 years ago

In an ideal world, I think the library should definitely be returning errors and not logging/printing anything. Recently, #81 made the necessary changes to return from pinger.Run() most of the errors that can occur.

There are some parts of the library where an error can be raised inside a goroutine; we've got a few ideas how to handle this such as an integer error counter, channel or maybe even a tomb but we'd welcome any other options/opinions - PRs being even better!

SuperQ commented 3 years ago

Another thing we could do is plumb in a logger, like we do here

lrstanley commented 3 years ago

I'd figure I'd make a comment even though the logger interface has already been implemented... Though, if trying to keep from breaking changes I'm not sure it matters much.

I don't think there is any case for this library having such a feature/implementation. Generally speaking (& especially with libraries like this), one shouldn't do any logging at all in the package. They should return errors that the user of the library can handle if they wish to do so (and log in the way they see fit).

  1. Logging is a rather opinionated thing, with many different implementations. When this library logs information, it's useless information since the end user can't do much with it. What failed? Why? Any metadata information? How do you propagate that up? etc. This is why the standard library doesn't have a log.Logger interface yet.
  2. Returning the errors (that include information about the failure), users can decide to log, track metrics, etc. Can't easily do that with a logger by itself (esp. if it's not a structured logger -- but supporting just a structured logger means those who don't have one, are also at a loss, thus another reason why it doesn't make much sense).
  3. Generally logging should be done by the destination author only, with few exceptions.

As mentioned by at least one other, IMO, using an error channel when it's blocking, where each error has the actual error message, but the metadata that users can pull from as well, and an error that contains an array of children errors if it's non-blocking. The API structure isn't the best for this at the moment, but IMO much better than including a large logger interface.

CHTJonas commented 3 years ago

In general I (and I think the other maintainers) agree with everything you've said but returning errors rather than logging would be a breaking change. That said, we're moving closer to the point of tagging v1 and starting work on v2.

In retrospect this was definitely closed prematurely - it was late at night when I merged #161 so I may have been a bit too hasty. #161 really just makes v1 suck a little bit less.