Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,8 @@ To check formatting and code quality, run `swift run BuildTool lint`. Run with `
- The aim of the [example app](README.md#example-app) is that it demonstrate all of the core functionality of the SDK. So if you add a new feature, try to add something to the example app to demonstrate this feature.
- If you add a new feature, try to extend the `IntegrationTests` tests to perform a smoke test of its core functionality.
- We should aim to make it easy for consumers of the SDK to be able to mock out the SDK in the tests for their own code. A couple of things that will aid with this:
- Describe the SDKs functionality via protocols (when doing so would still be sufficiently idiomatic to Swift).
- When defining a `struct` that is emitted by the public API of the library, make sure to define a public memberwise initializer so that users can create one to be emitted by their mocks. (There is no way to make Swifts autogenerated memberwise initializer public, so you will need to write one yourself. In Xcode, you can do this by clicking at the start of the type declaration and doing Editor → Refactor → Generate Memberwise Initializer.)
- Describe the SDK's functionality via protocols (when doing so would still be sufficiently idiomatic to Swift).
- When defining a `struct` that is emitted by the public API of the library, make sure to define a public memberwise initializer so that users can create one to be emitted by their mocks. (There is no way to make Swift's autogenerated memberwise initializer public, so you will need to write one yourself. In Xcode, you can do this by clicking at the start of the type declaration and doing Editor → Refactor → Generate Memberwise Initializer.)
- When writing code that implements behaviour specified by the Chat SDK features spec, add a comment that references the identifier of the relevant spec item.
- The SDK isolates all of its mutable state to the main actor. Stateful objects should be marked as `@MainActor`.
- Avoid the usage of existential types (`any FooProtocol`) in the public API; favour protocol associated types combined with opaque types (`some FooProtocol`).
Expand Down Expand Up @@ -90,7 +90,7 @@ I hope that as we understand more about Swift concurrency, we'll have a better u

When writing unit tests, there are times that we need to access internal state of a type. To enable this, we might expose this state at an `internal` access level so that it can be used by the unit tests. However, we want to make it clear that this state is being exposed _purely_ for the purposes of testing that class, and that it should not be used by other components of the SDK.

So, when writing an API which has `internal` access level purely to enable it to be called by the tests, prefix this APIs name with `testOnly_`. For example:
So, when writing an API which has `internal` access level purely to enable it to be called by the tests, prefix this API's name with `testOnly_`. For example:
Comment thread
lawrence-forooghian marked this conversation as resolved.

```swift
private let realtime: any RealtimeClientProtocol
Expand All @@ -104,7 +104,7 @@ private let realtime: any RealtimeClientProtocol

A couple of notes:

- Using a naming convention will allow us to verify that APIs marked `testsOnly` are not being used inside the SDK; well do this in #70.
- Using a naming convention will allow us to verify that APIs marked `testsOnly` are not being used inside the SDK; we'll do this in #70.
- I believe that we should be able to eliminate the boilerplate of re-exposing a `private` member as a `testsOnly` member (as exemplified above) using a macro (called e.g. `@ExposedToTests`), but my level of experience with macros is insufficient to be confident about being able to do this quickly, so have deferred it to #71.

#### Attributing tests to a spec point
Expand Down Expand Up @@ -135,11 +135,11 @@ func test3 { … }
```

```swift
// @specPartial CHA-EX1h4 - Tests that we retry, but not the retry attempt limit because weve not implemented it yet
// @specPartial CHA-EX1h4 - Tests that we retry, but not the retry attempt limit because we've not implemented it yet
func test4 { … }
```

You can run `swift run BuildTool spec-coverage` to generate a report about how many spec points have been implemented and/or tested. This script is also run in CI by the `spec-coverage` job. This script will currently only detect a spec point attribution tag if its written exactly as shown above; that is, in a `//` comment with a single space between each component of the tag.
You can run `swift run BuildTool spec-coverage` to generate a report about how many spec points have been implemented and/or tested. This script is also run in CI by the `spec-coverage` job. This script will currently only detect a spec point attribution tag if it's written exactly as shown above; that is, in a `//` comment with a single space between each component of the tag.

#### Marking a spec point as untested

Expand Down
4 changes: 2 additions & 2 deletions Example/AblyChatExample/Mocks/MockSubscriptionStorage.swift
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import Ably
import AblyChat

// This is copied from ably-chats internal class `SubscriptionStorage`.
// This is copied from ably-chat's internal class `SubscriptionStorage`.
@MainActor
class MockSubscriptionStorage<Element: Sendable> {
@MainActor
Expand Down Expand Up @@ -59,7 +59,7 @@ class MockSubscriptionStorage<Element: Sendable> {
}
}

// This is copied from ably-chats internal class `StatusSubscriptionStorage`.
// This is copied from ably-chat's internal class `StatusSubscriptionStorage`.
@MainActor
class MockStatusSubscriptionStorage<Element: Sendable> {
@MainActor
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ internal protocol InternalRealtimeChannelsProtocol: AnyObject, Sendable {

/// Expresses the requirements of the object returned by ``InternalRealtimeChannelsProtocol/get(_:options:)``.
///
/// We choose to mark the channels mutable state as `async`. This is a way of highlighting at the call site of accessing this state that, since `ARTRealtimeChannel` mutates this state on a separate thread, its possible for this state to have changed since the last time you checked it, or since the last time you performed an operation that might have mutated it, or since the last time you recieved an event informing you that it changed. To be clear, marking these as `async` doesnt _solve_ these issues; it just makes them a bit more visible. Well decide how to address them in https://github.com/ably-labs/ably-chat-swift/issues/49.
/// We choose to mark the channel's mutable state as `async`. This is a way of highlighting at the call site of accessing this state that, since `ARTRealtimeChannel` mutates this state on a separate thread, it's possible for this state to have changed since the last time you checked it, or since the last time you performed an operation that might have mutated it, or since the last time you recieved an event informing you that it changed. To be clear, marking these as `async` doesn't _solve_ these issues; it just makes them a bit more visible. We'll decide how to address them in https://github.com/ably-labs/ably-chat-swift/issues/49.
Comment thread
lawrence-forooghian marked this conversation as resolved.
@MainActor
internal protocol InternalRealtimeChannelProtocol: AnyObject, Sendable {
associatedtype Proxied: RealtimeChannelProtocol
Expand Down
2 changes: 1 addition & 1 deletion Sources/AblyChat/ChatClient.swift
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ public struct ChatClientOptions: Sendable {
self.logLevel = logLevel
}

/// Used for comparing these instances in tests without having to make this Equatable, which Im not yet sure makes sense (well decide in https://github.com/ably-labs/ably-chat-swift/issues/10)
/// Used for comparing these instances in tests without having to make this Equatable, which I'm not yet sure makes sense (we'll decide in https://github.com/ably-labs/ably-chat-swift/issues/10)
///
/// - Warning: Both set of options must have a `nil` `logHandler` (we can't compare `LogHandler` for equality because its underlying logger is not class-bound).
internal func isEqualForTestPurposes(_ other: ChatClientOptions) -> Bool {
Expand Down
2 changes: 1 addition & 1 deletion Sources/AblyChat/Connection.swift
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ public extension Connection {
* The different states that the connection can be in through its lifecycle.
*/
public enum ConnectionStatus: Sendable {
// (CHA-CS1a) The INITIALIZED status is a default status when the realtime client is first initialized. This value will only (likely) be seen if the realtime client doesnt have autoconnect turned on.
// (CHA-CS1a) The INITIALIZED status is a default status when the realtime client is first initialized. This value will only (likely) be seen if the realtime client doesn't have autoconnect turned on.

/**
* A temporary state for when the library is first initialized.
Expand Down
2 changes: 1 addition & 1 deletion Sources/AblyChat/ErrorInfo.swift
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ public struct ErrorInfo: Error, CustomStringConvertible {

/// Overrides the default implementation of this property that Foundation provides for the `Error` protocol.
///
/// Foundation's default implementation is not very useful; it just returns "The operation couldnt be completed. (AblyChat.ErrorInfo error 1.)" for all errors.
/// Foundation's default implementation is not very useful; it just returns "The operation couldn't be completed. (AblyChat.ErrorInfo error 1.)" for all errors.
public var localizedDescription: String {
description
}
Expand Down
2 changes: 1 addition & 1 deletion Sources/AblyChat/Errors.swift
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ internal enum InternalError: Error {

/// A specific error thrown by the internals of the Chat SDK.
///
/// This type exists in addition to ``ErrorCode`` to allow us to attach metadata which can be incorporated into the errors `message` and `cause`.
/// This type exists in addition to ``ErrorCode`` to allow us to attach metadata which can be incorporated into the error's `message` and `cause`.
internal enum InternallyThrown {
case other(Other)
case inconsistentRoomOptions(requested: RoomOptions, existing: RoomOptions)
Expand Down
22 changes: 11 additions & 11 deletions Sources/AblyChat/JSONValue.swift
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ public typealias JSONObject = [String: JSONValue]

/// A JSON value (where "value" has the meaning defined by the [JSON specification](https://www.json.org)).
///
/// `JSONValue` provides a type-safe API for working with JSON values. It implements Swifts `ExpressibleBy*Literal` protocols. This allows you to write type-safe JSON values using familiar syntax. For example:
/// `JSONValue` provides a type-safe API for working with JSON values. It implements Swift's `ExpressibleBy*Literal` protocols. This allows you to write type-safe JSON values using familiar syntax. For example:
///
/// ```swift
/// let jsonValue: JSONValue = [
Expand Down Expand Up @@ -148,8 +148,8 @@ internal extension JSONValue {
///
/// Specifically, `ablyCocoaData` can be:
///
/// - a non-`nil` value of `ARTBaseMessage`s `data` property
/// - an element of `ARTHTTPPaginatedResult`s `items` array
/// - a non-`nil` value of `ARTBaseMessage`'s `data` property
/// - an element of `ARTHTTPPaginatedResult`'s `items` array
init(ablyCocoaData: Any) {
switch ablyCocoaData {
case let dictionary as [String: Any]:
Expand All @@ -175,9 +175,9 @@ internal extension JSONValue {
}
}

/// Creates a `JSONValue` from an ably-cocoa deserialized JSON message extras object. Specifically, `ablyCocoaExtras` can be a non-`nil` value of `ARTBaseMessage`s `extras` property.
/// Creates a `JSONValue` from an ably-cocoa deserialized JSON message extras object. Specifically, `ablyCocoaExtras` can be a non-`nil` value of `ARTBaseMessage`'s `extras` property.
static func objectFromAblyCocoaExtras(_ ablyCocoaExtras: any ARTJsonCompatible) -> [String: JSONValue] {
// (This is based on the fact that, in reality, I believe that `extras` is always a JSON object; see https://github.com/ably/ably-cocoa/issues/2002 for improving ably-cocoas API to reflect this)
// (This is based on the fact that, in reality, I believe that `extras` is always a JSON object; see https://github.com/ably/ably-cocoa/issues/2002 for improving ably-cocoa's API to reflect this)

let jsonValue = JSONValue(ablyCocoaData: ablyCocoaExtras)
guard case let .object(jsonObject) = jsonValue else {
Expand All @@ -192,9 +192,9 @@ internal extension JSONValue {
///
/// Specifically, the value of this property can be used as:
///
/// - `ARTBaseMessage`s `data` property
/// - the `data` argument thats passed to `ARTRealtime`s `request(…)` method
/// - the `data` argument thats passed to `ARTRealtime`s `publish(…)` method
/// - `ARTBaseMessage`'s `data` property
/// - the `data` argument that's passed to `ARTRealtime`'s `request(…)` method
/// - the `data` argument that's passed to `ARTRealtime`'s `publish(…)` method
var toAblyCocoaData: Any {
switch self {
case let .object(underlying):
Expand All @@ -218,9 +218,9 @@ internal extension JSONObject {
///
/// Specifically, the value of this property can be used as:
///
/// - `ARTBaseMessage`s `data` property
/// - the `data` argument thats passed to `ARTRealtime`s `request(…)` method
/// - the `data` argument thats passed to `ARTRealtime`s `publish(…)` method
/// - `ARTBaseMessage`'s `data` property
/// - the `data` argument that's passed to `ARTRealtime`'s `request(…)` method
/// - the `data` argument that's passed to `ARTRealtime`'s `publish(…)` method
var toAblyCocoaDataDictionary: [String: Any] {
mapValues(\.toAblyCocoaData)
}
Expand Down
8 changes: 4 additions & 4 deletions Sources/AblyChat/Logging.swift
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ public enum LogLevel: Sendable, Comparable {

/// A reference to a line within a source code file.
internal struct CodeLocation: Equatable {
/// A file identifier in the format used by Swifts `#fileID` macro. For example, `"AblyChat/Room.swift"`.
/// A file identifier in the format used by Swift's `#fileID` macro. For example, `"AblyChat/Room.swift"`.
internal var fileID: String
/// The line number in the source code file referred to by ``fileID``.
internal var line: Int
Expand All @@ -67,7 +67,7 @@ internal protocol InternalLogger: Sendable {
}

extension InternalLogger {
/// A convenience logging method that uses the call sites #file and #line values.
/// A convenience logging method that uses the call site's #file and #line values.
public func log(message: String, level: LogLevel, fileID: String = #fileID, line: Int = #line) {
let codeLocation = CodeLocation(fileID: fileID, line: line)
log(message: message, level: level, codeLocation: codeLocation)
Expand Down Expand Up @@ -105,12 +105,12 @@ internal final class DefaultInternalLogger: InternalLogger {
return
}

// I dont yet know what `context` is for (will figure out in https://github.com/ably-labs/ably-chat-swift/issues/8) so passing nil for now
// I don't yet know what `context` is for (will figure out in https://github.com/ably-labs/ably-chat-swift/issues/8) so passing nil for now
logHandler.simple.log(message: "(\(codeLocation.fileID):\(codeLocation.line)) \(message)", level: level)
Comment thread
lawrence-forooghian marked this conversation as resolved.
}
}

/// The logging backend used by ``DefaultInternalLogHandler`` if the user has not provided their own. Uses Swifts `Logger` type for logging.
/// The logging backend used by ``DefaultInternalLogHandler`` if the user has not provided their own. Uses Swift's `Logger` type for logging.
internal final class DefaultSimpleLogHandler: LogHandler.Simple {
private let logger = Logger()

Expand Down
2 changes: 1 addition & 1 deletion Sources/AblyChat/Message.swift
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ public extension Message {
* - Returns: A new message instance with the event applied.
*/
func with(_ summaryEvent: MessageReactionSummaryEvent) throws(ErrorInfo) -> Self {
// (CHA-M11e) For MessageReactionSummaryEvent, the method must verify that the summary.messageSerial in the event matches the messages own serial. If they dont match, an error with code 40000 and status code 400 must be thrown.
// (CHA-M11e) For MessageReactionSummaryEvent, the method must verify that the summary.messageSerial in the event matches the message's own serial. If they don't match, an error with code 40000 and status code 400 must be thrown.
guard serial == summaryEvent.messageSerial else {
throw InternalError.internallyThrown(.cannotApplyEventForDifferentMessage).toErrorInfo()
}
Expand Down
Loading