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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Alamofire 5]: Update all inline API documentation. #2845

Merged
merged 41 commits into from Jun 29, 2019

Conversation

@jshier
Copy link
Contributor

commented Jun 15, 2019

Issue Link 馃敆

#2753

Goals 鈿斤笍

This PR brings our Jazzy documentation coverage from 78% to 98% by adding missing documentation and refactoring much of the existing documentation for modern styles.

Implementation Details 馃毀

This PR also brings some smaller changes:

  • DownloadRequest.cancel() no longer produces resumeData. Using cancel(byProducingResumeData:) will do that.
  • responseDecodable now takes a type parameter to make generic usage easier. This was recommended by Apple engineers in the lab. Should be integrating with Combine easier, in addition to general usage.

Testing Details 馃攳

DownloadRequest tests were updated now that cancel() doesn't produce resume data. A test was added for the new responseDecodable type parameter.

@jshier jshier requested a review from cnoon Jun 15, 2019

jshier added some commits Jun 17, 2019

Show resolved Hide resolved Source/AFResult.swift
@discardableResult
public override func cancel() -> Self {
return cancel(optionallyProducingResumeData: nil)

This comment has been minimized.

Copy link
@jshier

jshier Jun 17, 2019

Author Contributor

I refactored cancel() to not trigger resume data and added cancel(byProducingResumeData:) and refactored them to both use the same cancellation method. We may also want a cancel() version that does produce resume data like before, but I'm not sure.

This comment has been minimized.

Copy link
@cnoon

cnoon Jun 26, 2019

Member

I think what you did here is what I would recommend. If you want resume data, you should probably explicitly say so. It's unfortunate that this is different than AF4, but with all the change already, I don't think this is a big deal.

Show resolved Hide resolved Tests/DownloadTests.swift
Show resolved Hide resolved Tests/ResponseTests.swift
Show resolved Hide resolved Source/ResponseSerialization.swift

jshier added some commits Jun 17, 2019

@cnoon cnoon self-assigned this Jun 26, 2019

@cnoon cnoon added the documentation label Jun 26, 2019

@cnoon cnoon added this to the 5.0.0-beta.7 milestone Jun 26, 2019

@cnoon
Copy link
Member

left a comment

@jshier this is such an awesome PR. I found a few nits throughout, but only a couple potential code changes. Thanks for taking all the time here to get all those cases written down. Great work!

Show resolved Hide resolved Source/AFError.swift Outdated
Show resolved Hide resolved Source/AFError.swift
Show resolved Hide resolved Source/AFError.swift
Show resolved Hide resolved Source/Alamofire.swift Outdated
Show resolved Hide resolved Source/Alamofire.swift
Show resolved Hide resolved Source/Session.swift Outdated
/// - cachedResponseHandler: `CachedResponseHandler` to be used by all `Request`s created by this instance.
/// `nil` by default.
/// - eventMonitors: Additional `EventMonitor`s used by the instance. Alamofire always adds a
/// `AlamofireNotifications` `EventMonitor` to the array passed here. `[]` by default.

This comment has been minimized.

Copy link
@cnoon

cnoon Jun 26, 2019

Member

This makes me wonder if AlamofireNotifications should be an internal EventMonitor and the fact that we're doing this is just an internal detail that shouldn't be noted in the docstrings. I'm also not sure that the CompositeEventHandler looks for duplicates. For example, if the AlamofireNotifications is public, is it intended to be passed in? Would we then have two of them running? Thoughts?

This comment has been minimized.

Copy link
@jshier

jshier Jun 27, 2019

Author Contributor

My original thought here is that users may want to be able to create Sessions without AlamofireNotifications, but I never got around to working out how to expose such functionality. If we don't want that capability, we could just make AlamofireNotifications an internal implementation detail.

This comment has been minimized.

Copy link
@cnoon

cnoon Jun 28, 2019

Member

Gotcha. I don't really see that being an issue myself and lean towards making them internal altogether. If we end up with requests to be able to disable that functionality, then we could always add that option in the future. Maybe follow that change up in a separate PR?

Show resolved Hide resolved Source/Session.swift
Show resolved Hide resolved Source/SessionDelegate.swift Outdated
Show resolved Hide resolved Tests/ResponseTests.swift

jshier added some commits Jun 26, 2019

@jshier
Copy link
Contributor Author

left a comment

I've updated and addressed all of the feedback. I've left a few discussions open so you can see the answers to questions.

I also added another cancellation method for DownloadRequest, please take a look.

Show resolved Hide resolved Source/Alamofire.swift
Show resolved Hide resolved Source/Alamofire.swift
Show resolved Hide resolved Source/MultipartFormData.swift
/// available.
///
/// - Returns: The instance.
public func cancel(producingResumeData shouldProduceResumeData: Bool) -> Self {

This comment has been minimized.

Copy link
@jshier

jshier Jun 27, 2019

Author Contributor

Added a method to cancel without a completion handler but still producing resume data. It's a bit redundant with cancel(), but we have to override that. If you can find a better way to call down into one cancellation implementation, that would be great. For now I just pass an empty closure in the true case.

This comment has been minimized.

Copy link
@cnoon

cnoon Jun 28, 2019

Member

I can't think of a better approach since you have to override the cancel API. A better option would be to have a default value of producingResumeData, but as you mentioned, you can't override cancel using that approach. I think what you have is the only option. Looks good.

@cnoon

cnoon approved these changes Jun 28, 2019

Copy link
Member

left a comment

Looks great @jshier!

/// available.
///
/// - Returns: The instance.
public func cancel(producingResumeData shouldProduceResumeData: Bool) -> Self {

This comment has been minimized.

Copy link
@cnoon

cnoon Jun 28, 2019

Member

I can't think of a better approach since you have to override the cancel API. A better option would be to have a default value of producingResumeData, but as you mentioned, you can't override cancel using that approach. I think what you have is the only option. Looks good.

@jshier jshier merged commit dfd0ac2 into master Jun 29, 2019

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@jshier jshier deleted the feature/inline-docs-update branch Jun 29, 2019

jshier added a commit that referenced this pull request Jul 19, 2019

[Alamofire 5]: Update all inline API documentation. (#2845)
* Rename internal Result extensions.

* Update Alamofire docs.

* Update AFError docs.

* Cleanup HTTPHeaders.

* ParameterEncoder doc update.

* Whitespace cleanup.

* Update all Request docs, update Destination and cancel handling.

* Refactor download cancellation for common method.

* Start Session documentation.

* Update Gems.

* Update Session queue ids.

* Continue Session docs.

* DataRequest and DownloadRequest docs.

* Add default value note.

* Continue request docs.

* Whitespace cleanup.

* Finish Session docs, update AF docs.

* Small doc cleanup.

* SessionDelegate cleanup.

* Update docs for NetworkReachabilityManager.

* Update docs for MultipartFormData.

* Remove OS check, as compiler warns.

* Update RedirectHandler docs.

* Update RequestInterceptor docs.

* Update ServerTrustEvaluation docs.

* ResponseSerialization doc updates.

* Misc. cleanup.

* Update Gems.

* Add docs for missed public API.

* Make cancel() on DownloadRequest not produce resumeData.

* Fix download events.

* Add missing fileManager parameter.

* Make event tests more explicit.

* Whitespace cleanup.

* ParameterEncoding -> ParameterEncoder

* Fix typo.

* Updates for PR feedback.

* Final PR fixes.

* Add extra line before Returns doc.

* Clean up download cancellation.

toomasr added a commit to toomasr/Alamofire that referenced this pull request Aug 19, 2019

[Alamofire 5]: Update all inline API documentation. (Alamofire#2845)
* Rename internal Result extensions.

* Update Alamofire docs.

* Update AFError docs.

* Cleanup HTTPHeaders.

* ParameterEncoder doc update.

* Whitespace cleanup.

* Update all Request docs, update Destination and cancel handling.

* Refactor download cancellation for common method.

* Start Session documentation.

* Update Gems.

* Update Session queue ids.

* Continue Session docs.

* DataRequest and DownloadRequest docs.

* Add default value note.

* Continue request docs.

* Whitespace cleanup.

* Finish Session docs, update AF docs.

* Small doc cleanup.

* SessionDelegate cleanup.

* Update docs for NetworkReachabilityManager.

* Update docs for MultipartFormData.

* Remove OS check, as compiler warns.

* Update RedirectHandler docs.

* Update RequestInterceptor docs.

* Update ServerTrustEvaluation docs.

* ResponseSerialization doc updates.

* Misc. cleanup.

* Update Gems.

* Add docs for missed public API.

* Make cancel() on DownloadRequest not produce resumeData.

* Fix download events.

* Add missing fileManager parameter.

* Make event tests more explicit.

* Whitespace cleanup.

* ParameterEncoding -> ParameterEncoder

* Fix typo.

* Updates for PR feedback.

* Final PR fixes.

* Add extra line before Returns doc.

* Clean up download cancellation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can鈥檛 perform that action at this time.