#273: Expose connectTimeout as a configuration option

[trasch]

This exposes the connectTimeout property in the configuration.

let config = PostgresConnection.Configuration(
   connection: .init(
     host: "localhost",
     port: 5432,
     connectTimeout: .seconds(5)
   ),
   authentication: .init(
     username: "my_username",
     database: "my_database",
     password: "my_password"
   ),
   tls: .disable
)

[fabianfett]

@trasch sorry for tkaing so long to come back to you!

There is one direct code comment, and would you be open to add a test case for it?

[fabianfett]

Suggested change

	public init(host: String, port: Int = 5432, connectTimeout: TimeAmount = .seconds(10)) {
	self.host = host
	self.port = port
	self.connectTimeout = connectTimeout
	public init(host: String, port: Int = 5432) {
	self.host = host
	self.port = port
	self.connectTimeout = .seconds(10)

We sadly can not add another parameter to the initializer and not be source breaking. To work around this all properties are vars by default. This way, if users want to override the value, they can just use the property after the init.

[trasch]

No problem, but I have to admit that I don't understand why this is source breaking... 🙂 Shouldn't a new parameter with a default value at the end of the parameter list compile everywhere without problems?

[fabianfett]

Yes, this API breaking change is very hard to see, but it occurs in this case:

Existing API:

struct Foo {
    init(bar: String) {

    }

    static func test() {
        let make = Foo.init(bar:)
        let foo = make("hello")
        // ...
    }
}

Adding a defaulted value breaks this code however:

struct Foo {
    init(bar: String, test: String = "foo") {

    }

    static func test() {
        let make = Foo.init(bar:) // <--- ❌ compiler error
        let foo = make("hello")
        // ...
    }
}

[trasch]

@fabianfett

would you be open to add a test case for it

I would, and maybe you can give me a hint if there is already some code somewhere that opens a blocking port with which I could test this?

[fabianfett]

@trasch after browsing around a bit, this seems to be quite hard to test... Sorry for making you look into it. Basically we would test nio functionality. If the CI passes, we should be good to go!

[codecov-commenter]

Codecov Report

Merging #276 (20698ea) into main (2ddc2e1) will increase coverage by 0.00%.
The diff coverage is 80.00%.

@@           Coverage Diff           @@
##             main     #276   +/-   ##
=======================================
  Coverage   43.90%   43.91%           
=======================================
  Files         115      115           
  Lines        9675     9678    +3     
=======================================
+ Hits         4248     4250    +2     
- Misses       5427     5428    +1     

Flag	Coverage Δ
unittests	43.91% <80.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...es/PostgresNIO/Connection/PostgresConnection.swift	16.99% <80.00%> (+0.33%)	⬆️

[trasch]

@fabianfett

@trasch after browsing around a bit, this seems to be quite hard to test... Sorry for making you look into it. Basically we would test nio functionality. If the CI passes, we should be good to go!

Indeed, testing this would basically be replicating NIOTSEndToEndTests.testBasicConnectionTimeout() from the NIOTransportServices library - which I found a minute after asking you for help... :-D

Thanks for reviewing this!

[fabianfett]

Awesome! Thanks for the contribution @trasch!