Breaking change to `Crystal::System::Socket#system_connect` timeout type

[nobodywasishere]

Bug Report

Related to #14368.

In #14643, Crystal::System::Socket#system_connect was changed to no longer automatically convert Number types (using .seconds) to Time::Spans, instead explicitly requiring them. This is a breaking change. To have this be deprecated instead, there should've been an additional method overload that did the automatic conversion with the @[Deprecated] annotation.

[Blacksmoke16]

Crystal::System is an internal implementation detail. So while this may be a breaking change, unless it affected some public API, it is allowed. Was how you discovered this using system_connect directly or?

[nobodywasishere]

system_connect is used by Socket#connect, so this is a breaking change for it and its subclasses (IPSocket, TCPSocket, UDPSocket, etc.)

[ysbaddaden]

We won't fix #system_connect since it's undocumented internal API, but there should indeed be a deprecated override method for Socket#connect(addr, timeout = nil, &) to not break the public API :+1:

[straight-shoota]

Thanks for reporting and fixing. Apparently this got lost in the heat of the moment.

[straight-shoota]

Actually, I believe the correct and direct fix would be to move the original conversion from #system_connect to #connect:

--- i/src/socket.cr
+++ w/src/socket.cr
@@ -110,6 +110,7 @@ class Socket < IO
   # Tries to connect to a remote address. Yields an `IO::TimeoutError` or an
   # `Socket::ConnectError` error if the connection failed.
   def connect(addr, timeout = nil, &)
+    timeout = timeout.seconds unless timeout.is_a? ::Time::Span | Nil
     result = system_connect(addr, timeout)
     yield result if result.is_a?(Exception)
   end

I've had this in my development branch, but it must've gotten lost somewhere during refactoring and rebasing.

After this we can move forward with adding type restricitons to timeout and deprecating the overload which is not Time::Span?.