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

Alamofire 5! #1810

Merged
merged 34 commits into from Aug 1, 2019

Conversation

@sunshinejr
Copy link
Member

commented Feb 18, 2019

No description provided.

@BasThomas

This comment has been minimized.

Copy link
Contributor

commented Feb 18, 2019

Ooh awesome, thanks for all the work recently, @sunshinejr!

@sunshinejr

This comment has been minimized.

Copy link
Member Author

commented Feb 18, 2019

@BasThomas thanks! It's still a long way to go, though!

@MoyaBot

This comment has been minimized.

Copy link

commented Feb 19, 2019

1 Warning
⚠️ Consider adding supporting documentation to this change. Documentation can be found in the docs directory.
3 Messages
📖 iOS: Executed 277 tests, with 0 failures (0 unexpected) in 13.040 (13.176) seconds
📖 tvOS: Executed 277 tests, with 0 failures (0 unexpected) in 13.329 (13.469) seconds
📖 macOS: Executed 277 tests, with 0 failures (0 unexpected) in 13.334 (13.455) seconds

Generated by 🚫 Danger

@codecov-io

This comment has been minimized.

Copy link

commented Feb 19, 2019

Codecov Report

Merging #1810 into development will increase coverage by 0.06%.
The diff coverage is 92.2%.

Impacted file tree graph

@@              Coverage Diff               @@
##           development   #1810      +/-   ##
==============================================
+ Coverage        92.13%   92.2%   +0.06%     
==============================================
  Files               25      26       +1     
  Lines              865     898      +33     
==============================================
+ Hits               797     828      +31     
- Misses              68      70       +2
Impacted Files Coverage Δ
Sources/Moya/Plugin.swift 75% <ø> (ø) ⬆️
Sources/Moya/MoyaProvider+Defaults.swift 78.26% <100%> (-1.74%) ⬇️
Sources/Moya/MoyaProvider+Internal.swift 98.57% <100%> (+1.55%) ⬆️
Sources/Moya/MoyaProvider.swift 91.66% <100%> (ø) ⬆️
Sources/Moya/Plugins/CredentialsPlugin.swift 100% <100%> (ø) ⬆️
Sources/Moya/RequestTypeWrapper.swift 72.72% <72.72%> (ø)
Sources/Moya/Moya+Alamofire.swift 84.21% <85%> (+1.6%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cf9ee28...c3ac3ac. Read the comment docs.

@sunshinejr

This comment has been minimized.

Copy link
Member Author

commented Feb 23, 2019

Update: it compiles, but I did a little workaround for willSend plugin method. I'm currently discussing if we could get a way of having willSend method provided by Alamofire, so gonna hold with this PR until that conversation is resolved.

@Dschee

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2019

@sunshinejr I just added two commits with an update to Alamofire 5 beta 3 and with also updating to Alamofire 5 when installing Moya via SwiftPM. I hope that's fine with you, I pushed since they were just small changes and this is a WIP anyways.

@sunshinejr

This comment has been minimized.

Copy link
Member Author

commented Mar 30, 2019

@Dschee yeah no worries, thanks!

@sunshinejr sunshinejr force-pushed the feature/alamofire_5 branch from 43209d6 to 5a9e0b7 May 2, 2019

@sunshinejr sunshinejr marked this pull request as ready for review Jul 4, 2019

@sunshinejr sunshinejr changed the title [WIP] Alamofire 5! Alamofire 5! Jul 4, 2019

sunshinejr added some commits Jul 4, 2019

@sunshinejr

This comment has been minimized.

Copy link
Member Author

commented Jul 4, 2019

@Moya/core-team Would anyone be so kind to check this one out? I'd love it if anyone could do a quick check on an existing app to see if everything works correctly.

sunshinejr added some commits Jul 5, 2019

@sunshinejr sunshinejr referenced this pull request Jul 9, 2019
@sunshinejr

This comment has been minimized.

Copy link
Member Author

commented Jul 27, 2019

@larryonoff, @LucianoPAlmeida, @pedrovereza, @Moya/contributors, anyone familiar with Alamofire/Moya core please take a look and let me know if you see any potential issues. Thanks!


/// Make the Alamofire Request type conform to our type, to prevent leaking Alamofire to plugins.
extension Request: RequestType { }

/// Represents Request interceptor type that can can modify/act on Request

This comment has been minimized.

Copy link
@LucianoPAlmeida

LucianoPAlmeida Jul 27, 2019

Member

Nit: typo can can :))

@LucianoPAlmeida
Copy link
Member

left a comment

Awesome work @sunshinejr \o/
Code wise LGTM, but I was not able to test this in an app yet.
Can give you feedback on that as soon as possible :))

}
}
}

extension DownloadRequest: Requestable {
internal func response(callbackQueue: DispatchQueue?, completionHandler: @escaping RequestableCompletion) -> Self {
return response(queue: callbackQueue) { handler in
completionHandler(handler.response, handler.request, nil, handler.error)
if let callbackQueue = callbackQueue {

This comment has been minimized.

Copy link
@LucianoPAlmeida

LucianoPAlmeida Jul 27, 2019

Member

Not related, but we are doing this if let callBack twice (here and on DataRequest), should we move this logic to response(queue: so we could just pass the callback and it could be handled there?

This comment has been minimized.

Copy link
@sunshinejr

sunshinejr Jul 28, 2019

Author Member

Ah yes it would be but the response(queue: is an Alamofire func and these two funcs are just helpers that enable queue as an optional queue instead of this if/else 😄

@@ -0,0 +1,67 @@
import Alamofire

internal struct PropertyListEncoding: ParameterEncoding {

This comment has been minimized.

Copy link
@LucianoPAlmeida

LucianoPAlmeida Jul 27, 2019

Member

Since this is internal, why all the properties and methods are public?

This comment has been minimized.

Copy link
@sunshinejr

sunshinejr Jul 28, 2019

Author Member

ah good catch, I just copied the encoding for testing as Alamofire removed it from version 5 😮

/// The manager for the session.
public let manager: Manager
/// The session for the session.
public let session: Session

This comment has been minimized.

Copy link
@larryonoff

larryonoff Jul 28, 2019

Contributor

The session for the session

I think this should be changed a bit, e.g. The session for the network session / Moya provider

This comment has been minimized.

Copy link
@sunshinejr

sunshinejr Jul 28, 2019

Author Member

oh damn didn't notice that🤦‍♂ nice catch!

This comment has been minimized.

Copy link
@sunshinejr

sunshinejr Jul 28, 2019

Author Member

Actually I think this doesn't really need a comment, seems self-explanatory so I will remove it for now.


/// Authenticates the request with an `NSURLCredential` instance.
func authenticate(usingCredential credential: URLCredential) -> Self
func authenticate(with credential: URLCredential) -> Self

This comment has been minimized.

Copy link
@larryonoff

larryonoff Jul 28, 2019

Contributor

just personal suggestion. what about replacing with with using?

This comment has been minimized.

Copy link
@sunshinejr

sunshinejr Jul 28, 2019

Author Member

I like it! But this is an Alamofire func and this is their naming change 😢

@sunshinejr

This comment has been minimized.

Copy link
Member Author

commented Jul 28, 2019

@larryonoff @LucianoPAlmeida thanks for the fast CR! I've updated the PR & added some comments as well.

@LucianoPAlmeida
Copy link
Member

left a comment

LGTM 💯

@sunshinejr

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2019

Alright, bumped Alamofire to 5.0.0-beta.7 as it had minimal changes - merging it!

@sunshinejr sunshinejr merged commit 286d0fe into development Aug 1, 2019

2 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: carthage_without_swiftlint_integration Your tests passed on CircleCI!
Details
@LucianoPAlmeida

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

Awesome @sunshinejr, now that this was merged maybe #1878 could get in too?
Also do you have plans for a pre-release(alpha/beta) of this one?

@sunshinejr

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2019

@LucianoPAlmeida Yeah, I'm in the process of merging #1878 and then releasing new alpha. Wanted #1880 as well but it needs a little bit more time so we will add it to the next pre-release.

@sunshinejr sunshinejr deleted the feature/alamofire_5 branch Aug 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.