Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide way to set cluster consistency #8

Merged
merged 5 commits into from
Nov 9, 2022
Merged

Conversation

yim-lee
Copy link
Member

@yim-lee yim-lee commented Nov 8, 2022

rdar://102086631

@yim-lee yim-lee requested a review from tomerd November 8, 2022 22:24
@yim-lee
Copy link
Member Author

yim-lee commented Nov 8, 2022

cc @rnro @glbrntt

Copy link

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

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

A couple of nits but looks good otherwise. Thanks for the quick fix!

@@ -90,7 +90,7 @@ public class CassandraClient: CassandraSession {
/// - logger: If `nil`, the client's default `Logger` is used.
///
/// - Returns: The resulting ``Rows``.
public func execute(statement: Statement, on eventLoop: EventLoop?, logger: Logger? = nil) -> EventLoopFuture<Rows> {
public func execute(statement: Statement, on eventLoop: EventLoop?, logger: Logger? = .none) -> EventLoopFuture<Rows> {
Copy link

Choose a reason for hiding this comment

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

completely subjective but IMO .none is less idiomatic than nil


public extension CassandraClient {
/// Consistency levels
enum Consistency {
Copy link

Choose a reason for hiding this comment

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

This should be public.

I'm also wondering if this should be a struct backed by an internal enum with a public static let per-case: users shouldn't need to switch over it and new "cases" can be added without breaking API.

Copy link

Choose a reason for hiding this comment

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

Relatedly: CassandraClient.Configuration should also be public (all the methods and functions are but the type isn't)

Copy link

Choose a reason for hiding this comment

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

Oh my mistake: both are defined in public extensions.

Copy link
Member Author

@yim-lee yim-lee Nov 9, 2022

Choose a reason for hiding this comment

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

Updating SwiftFormat rule to prevent this sort of confusion. Amended a245c86

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm also wondering if this should be a struct backed by an internal enum with a public static let per-case

I understand your point, but I doubt users would switch over this particular enum.

Sources/CassandraClient/Configuration.swift Outdated Show resolved Hide resolved
Sources/CassandraClient/Configuration.swift Show resolved Hide resolved
@yim-lee yim-lee merged commit f42a51d into main Nov 9, 2022
@yim-lee yim-lee deleted the cluster-consistency branch November 9, 2022 23:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants