Skip to content

Conversation

@fabianfett
Copy link
Member

No description provided.

@fabianfett fabianfett added the 🆕 semver/minor Adds new public API. label Jul 3, 2023
/// - Parameters:
/// - connectionInitialDatabase: The optional database index to initially connect to. The default is `nil`.
/// Redis by default opens connections against index `0`, so only set this value if the desired default is not `0`.
/// Redis by default opens connections against index `0`, so only set this value if the desired default is not `0`.
Copy link
Member

Choose a reason for hiding this comment

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

Was this intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes. nicer format.

Copy link
Member

@Joannis Joannis left a comment

Choose a reason for hiding this comment

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

Check the breaking change. The rest looks good

connectionUsername: String? = nil,
connectionPassword: String? = nil,
connectionDefaultLogger: Logger? = nil,
tcpClient: ClientBootstrap? = nil
Copy link
Member

Choose a reason for hiding this comment

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

I think this will break, as we now have two instances of .init() where all arguments are defaulted to nil

}
}

private func send0(
Copy link
Member

Choose a reason for hiding this comment

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

❤️

/// - `defaultLogger`: The optional prototype logger to use as the default logger instance when generating logs from the connection.
/// If one is not provided, one will be generated. See `RedisLogging.baseConnectionLogger`.
/// - Throws: `RedisConnection.Configuration.ValidationError` if invalid arguments are provided.
public init(
Copy link
Member

Choose a reason for hiding this comment

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

Actually, isn't this the same scenario? Also two cases where a public init(address: ...) would be ambiguous?

@fabianfett fabianfett merged commit 89a29d4 into swift-server:main Jul 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🆕 semver/minor Adds new public API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants