Skip to content
This repository has been archived by the owner on Mar 11, 2022. It is now read-only.

Follow API naming Guidelines #96

Closed
rhyshort opened this issue Jul 6, 2016 · 2 comments
Closed

Follow API naming Guidelines #96

rhyshort opened this issue Jul 6, 2016 · 2 comments
Assignees
Milestone

Comments

@rhyshort
Copy link
Member

rhyshort commented Jul 6, 2016

Some APIs do not follow the API naming Guidelines for Swift 3. Such as the enum Stale which has the elements Ok and UpdateAfter in Swift 3 they should be named ok and updateAfter respectively. There are probably other APIs which do not match the convention, we need to audit and propose replacements for each API that does not follow the guidelines.

The guidelines can be found here.

@rhyshort rhyshort added this to the 0.4.0 milestone Jul 6, 2016
@rhyshort rhyshort self-assigned this Jul 11, 2016
@rhyshort
Copy link
Member Author

Swift API Guidlines and SwiftCloudant

Along with Swift 3, there are official API guidelines approved by the Swift community community via the swift evolution process.

SwiftCloudant currently does not follow the guidelines that are laid out by the community. The most common violation of the guidelines are enum capitalisation, for example, .Ok should be .ok.
This leads to our APIs being difficult to work with and do not behave in the manor that the user would expect.

Our aim should be to make the API as expressive as possible and behave has expected. This may mean departing form the CouchDB way of naming parameters to make their purpose clearer. The example of that
would be updateSeq, which determines if the last update sequence number should be included in the response. It does not provide a sequence number to be used in the request.

General

Changes discussed here will affect multiple classes, protocols or extensions.

There are some potential issues with the API, with Swift's explicit nullablity constructs, it is odd that we allow the the assigning of nil to options which are required. For example
docId is always required for operations operating on documents or on attachments.

To simplify documentation, validation logic and behave as the user would expect, should required parameters become required for initialization and become explicitly non-null? This goes beyond
just document IDs and covers each of the parameters which are required as per the validation rules for the operation. databaseName should be made non-optional since all
operations which operate on databases will require the name of the database to be set.

The naming of parameters are abbreviated, perhaps the abbreviations should be changed to make more sense. For example the docId should at least be documentID or docID. However
with some operations such as PutDocumentOperation using just id would have make sense, especially since we currently use body to refer to the document body. It would also tie
the parameters more closely to the context they appear in. However it does have the potential to cause confusion with the different naming on different operations. For example:

public class GetDocumentOperation : CouchOperation, CouchDatabaseOperation, JSONOperation {

    // ...

    public init(id: String) {
        // ....
    }

    // ...
}

public class GetAttachmentOperation : CouchOperation, CouchDatabaseOperation, DataOperation {

    // ...

    public init(documentID: String, name: String) {
        // ...
    }

    // ...
}

With GetDocumentOperation it is possible that the user will infer that the id parameter is the ID of the document they are going to get from the server without reading the documentation. However
with the parameter called documentID for the GetAttachmentOperation it is possible (although unlikely) the user may expect the parameter to be the same. However the simplicity of using the id as
the parameter label for the init method for document operations.

// more abbreviations

The method convertJson(key: AnyObject) -> String throws should be renamed the jsonValue(of key: AnyObject) -> String throws this will fit within the design guidelines from the community. The method name would then indicate that it
will be returning the converted value as a JSON string. The usage example makes this case the clearest:

let viewKey = "api"
let jsonViewKey = jsonValue(of: viewKey)

With the above it is plain to see what the intent of the method is without looking at the documentation. While we are also here we should be making it possible to use the native swift types directly with this method. So
the type of the parameter of key should become Any which will allow the use of value types without attempting to cast them to their bridged counterpart. The final method signature should be jsonValue(of key: Any) -> String throws

CouchDBClient

Not sure about these changes.

Having to write nil several times to indicate there is not a username or password to be used for this client can be tedious. To better make it to work with an avoid writing nil for the username and the password parameters
a new init method should be introduced.

public init(url: URL)

The above will call down into the designated constructor with the username and password set to nil. Also to simplify the init logic for the CouchDBClient for when auth information is present a second constructor should be created:

public init(url: URL, username: String, password: String)

this would make it clear that a username and password should both be provided.

/Not sure about these changes

The adding operations to the queue to be executed needs improvement. It can be confusing which kind of operation you have when faced the the method signatures:

public func add(operation: CouchOperation) -> Operation {}

public func add(operation: Operation) -> Void {}

There is no easy solution for this, however we could mitigate the confusion for users by replacing those methods with

public func add(couchOperation: CouchOperation) -> Operation

public func add(operation: Operation) -> Void

It reduces cognitive burden around if you'll get an object returned or not, However documentation should make it clear with the case of the CouchOperation variant, that the
returned operation has already been added to the queue and as such it should be used in the add(operation:) variant.

CouchOperation

  • func callCompletionHandler(response: Any?, httpInfo: HTTPInfo?, error: ErrorProtocol?) should be translated
    to complete(response: Any?, httpInfo: HTTPInfo?, error: ErrorProtocol) it will make it clear at the point
    of use that this completes the operation and no more processing of the response should be taking place. As a result the func callCompletionHandler(error: ErrorProtocol) method
    should be changed to be func complete(error: ErrorProtocol?)
guard error == nil, let httpInfo = httpInfo
else {
    self.complete(error: error!)
    return
} 
self.complete(response: response, httpInfo: info, error: nil)

JsonOperation

  • The JsonOperation protocol needs to be renamed to JSONOperation, this is in keeping with the expected capitalisation.
  • associated type needs to be renamed from Json to JSON, again this is to be in keeping with the expected capitalisation.

CreateDatabaseOperation

CreateDatabaseOperation exposes it's database name as an property an an init method which takes no arguments, now the property needs to be set before the operation can be completed. Perhaps the type for the database name needs to be changed to String rather than String? since this will remove the need to validate this property
and reduce the need to use ! operator to unwrap the db name when the endpoint property is requested. This will also take the surprise out when validation fails for the first time, perhaps with the user expecting a name
to be generated since we did not require one at the time the operation was constructed.

CreateJsonQueryIndexOperation

Class needs to be renamed to CreateJSONQueryIndexOperation since JSON is an abbreviation.

Properties.

indexName could be renamed to name since it should be obvious that it is the name of the index.
databaseName and fields should probably be not be optionals and force them to be set via the init method.
designDoc might not be as clear at the point of use as it could be, perhaps renaming it to designDocument or designDocumentID, might be a better way forward.

CreateTextQueryIndexOperation

Properties

Again indexname could be renamed to name.

  • databaseName, is the only required property so making it a init requirement will make the operation easier to interact with.

TextIndexFieldType

This should be relocated from it's current top level and move to be within the TextIndexField struct. It
should then also be renamed to Type. This Short name conforms better with the API guidelines.

The vase entries need to be renamed to be lower case. The cases should be boolean, string and number.

DeleteAttachmentOperation

Change type of databaseName, docId and revId to String from String? introduce new init method to
provide these required options.

DeleteQueryIndexOperation

indexName -> name index portion is not required.
IndexType -> DeleteQueryIndexOperation.Type
IndexType.JSON -> Type.json
IndexType.Text -> Type.text

FindDocumentsOperation

SortDirection -> Sort.Direction
SortDirection.Asc -> Sort.Direction.asc
SortDirection.Desc -> Sort.Direction.desc

Mango Operation

transform(sortArray:) -> .transformed() however this is not currently possible with the current generics
implemented in swift. Some investigation required to see if this will be possible in swift 3.

GetDocumentOperation

revs -> includeRevisions this is a bit of a departure from the couchDb api, however it makes it clearer
what is happening compared to the current revs option.

Operation

Errors -> Operation.Error
Errors.ValidationFailed -> .Error.validationFailed
Errors.UnexpectedJSONFormat -> .Error.unexpectedJSONFormat
Errors.HTTP -> Error.http

QueryViewOperation

viewName -> name
updateSeq is a bit of a weird one, on its own, you'd think it would be the Seqeunce number however it isn't
its whether or not to include the sequence number of the last update, as a result perhaps it should be renamed
to includeUpdateSequence or includeUpdateSeq this would it make it more clear that you are asking for it to
be included in the reponse and not something you are including in the request.

OperationRequestBuilder

buildRequest -> makeRequest since its more of a factory, it should be cahnegd from build to request it
might also be worth making it a class method such as class func makeRequest(from operation: HTTPOperation)
rather than going through the init chain and retaining the operation.

HTTPInterceptor

interceptRequest(ctx: HTTPInterceptorContext) -> HTTPInterceptorContext -> interceptRequest(in context: HTTPInterceptorContext)
interceptResponse(ctx: HTTPInterceptorContext) -> HTTPInterceptorContext -> interceptRequest(in context: HTTPInterceptorContext)

ViewOperation

Stale.Ok -> Stale.ok
Stale.UpdateAfter -> Stale.updateAfter

keys and key and potentially confusing, perhaps we should move this to just be keys and document that if users want to match one key they have a single key in the array.

updateSeq see previous discussion.

generateParams -> makeParameters, since this method is more like a factory method, changing it to be makeParameters makes more sense for the context.

@rhyshort
Copy link
Member Author

So the above gives me view on what needs to be changed to match the guidelines, anyone have any thoughts? @ricellis

rhyshort added a commit that referenced this issue Jul 28, 2016
Renaming and reworking of all externally faceing APIs and some internally facing
APIs to match the API guidlines. See issue #96 for full details on the
exact changes.

Resolves #96
rhyshort added a commit that referenced this issue Jul 28, 2016
Renaming and reworking of all externally faceing APIs and some internally facing
APIs to match the API guidlines. See issue #96 for full details on the
exact changes.

Resolves #96
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant