Skip to content

Conversation

@kmahar
Copy link
Contributor

@kmahar kmahar commented Jan 27, 2020

Most of this PR is just slightly reformatting docstrings to clarify that futures fail with particular errors now, rather than the errors being thrown.

This excludes the docstrings for cursors and change streams, and methods that return cursors and change streams, as I think you started on those already, but if you think I should pick up any of that while you work on implementing those types let me know.

@kmahar kmahar requested a review from patrickfreed January 27, 2020 17:36
* - Throws:
* - `LogicError` if the provided session is inactive.
* - `EncodingError` if an error is encountered while encoding the options to BSON.
* - Returns: An `EventLoopFuture<[DatabaseSpecification]>`. On success, the future contains an array of the
Copy link
Contributor

Choose a reason for hiding this comment

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

According to the "Multiline Elements" section of Apple's markup docs, these blocks can begin on the next line down from their delimiters (i.e. - Returns in this case), and they also can be split into multiple paragraphs by inserting single empty lines between sections of text. I personally think its a little clearer if the return content starts on its own line below - Returns, and that the error information is separated into its own paragraph.

e.g.

* - Returns:
* An `EventLoopFuture<T?>` containing the next `T` in this cursor, an error if one occurred, or `nil` if
* there was no data.
*
* If the future evaluates to an error, it is likely one of the following:
* - `CommandError` if an error occurs while fetching more results from the server.
* - `LogicError` if this function is called after the cursor has died.
* - `LogicError` if this function is called and the session associated with this cursor is inactive.
* - `DecodingError` if an error occurs decoding the server's response.
*/

The way this is actually written likely could be improved, but I think the overall format is a bit clearer than a single paragraph. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sgtm, updated all of them

@kmahar kmahar force-pushed the SWIFT-704/update-async-docstrings branch from 3ff8b03 to e0d168a Compare January 31, 2020 23:39
@kmahar kmahar force-pushed the SWIFT-704/update-async-docstrings branch from dce43f1 to 65cb402 Compare February 4, 2020 18:27
Copy link
Contributor

@patrickfreed patrickfreed left a comment

Choose a reason for hiding this comment

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

one little suggestion, otherwise LGTM

* - sessionBody: A closure which takes in a `ClientSession` and returns an `EventLoopFuture<T>`.
*
* - Returns:
* An `EventLoopFuture<T>`, the return value of the user-provided closure.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could say "On success, the future contains ..." similar to the other docstrings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opted to not do that here because we actually (from the user's perspective) directly return the return value of their closure (which happens to be a future because of how we've specified the type of sessionBody). regardless of whether that succeeds or fails we will directly return it

@kmahar kmahar merged commit 478df3f into master Feb 6, 2020
@kmahar kmahar deleted the SWIFT-704/update-async-docstrings branch February 6, 2020 00:52
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