Remove panics

[zergon321]

Panicking is not a good way to handle emergency situations. It's better to use errors instead. Especially in your case, when you need to check if connection or callback is not nil. Panicking is unnecessary here.

[codecov-io]

Codecov Report

Merging #1 into master will increase coverage by 0.08%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master       #1      +/-   ##
==========================================
+ Coverage   95.87%   95.95%   +0.08%     
==========================================
  Files           3        3              
  Lines          97       99       +2     
==========================================
+ Hits           93       95       +2     
  Misses          2        2              
  Partials        2        2

Impacted Files	Coverage Δ
Packet.go	86.2% <ø> (ø)	⬆️
Stream.go	100% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 929be90...2ddf6a4. Read the comment docs.

[akyoto]

I can see the reasoning behind this PR. However, let me explain my personal view on the decision:

Calling SetConnection with a nil connection is defined behavior and it will panic to reveal mistakes in your application code. SetConnection is not meant to be an application's connection testing code.

An explicit if conn == nil handler in your application code is required. Panicking will catch bugs during development early and prevent using a nil connection for further initialization in application code and cause less damage.

In Haskell terms, the type argument of SetConnection is net.Conn, not Maybe net.Conn.
In TypeScript terms it would be net.Conn, not net.Conn | null.
Go's type system lacks the ability to denote that a pointer argument cannot be nil, therefore a panic is appropriate in this case.

I personally think an update to the documentation, to make it 100% clear that you must do your own connection checks on the application level, would be the best.

conn := accept()

if conn == nil {
	log("something went wrong here")
	return
}

stream.SetConnection(conn)

What you are suggesting is:

conn := accept()
err := stream.SetConnection(conn)

if err != nil {
	log(err)
	return
}

But SetConnection cannot fail. Having an error return value on SetConnection would make sense if the type argument was net.Conn | nil. It is defined to only accept non-nil parameters. There are also no other possible error sources because it's just a variable assignment.

You need to adhere to the rules of a library/package. As an example, take any function that is not thread-safe. Calling a thread-unsafe function in a multi-threaded context will cause data races and it might not even give you the luxury of causing a panic to find these mistakes early. Thread-unsafe functions do not return an error value in a multi-threaded context. The user needs to adhere to the calling convention, the established rules for using it which are defined above the type system.

I'm leaving this PR open for further comments because it is a grey area and not a simple decision.

My current decision boils down to improving the documentation.

[akyoto]

See Is function parameter validation using errors a good pattern in Go?.

The author writes about panics in parameter checks:

It might make errors more obvious—the execution fails in the API function, not somewhere deeper down the call stack—but adds unnecessary cruft to these API functions. So yes, this is opinionated but my subjective opinion is: to let it just crash is OK in Go—you'll get a descriptive stack trace.

My opinion (disagreeing with the author here) is that a panic in the API is better than a random nil pointer panic somewhere deep down the call stack.