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]: AlamofireExtended extension point. #2758

Merged
merged 1 commit into from Mar 27, 2019

Conversation

Projects
None yet
2 participants
@jshier
Copy link
Contributor

commented Mar 24, 2019

Issue Link 馃敆

#2750

Goals 鈿斤笍

This PR adds the AlamofireExtended protocol and AlamofireExtension type to enable .af extension points on any conforming type. This allows us to put all of our extensions of types we don't own behind a single entry point.

Implementation Details 馃毀

Types conforming to AlamofireExtended receive the .af static and instance properties. Extending AlamofireExtension where ExtendedType is necessary to add functionality to those extension points on particular types. Our extensions of various Security types, as well as URLSessionConfiguration, have been updated to use this pattern.

Testing Details 馃攳

Tests of the extended API have been updated, but no specific tests have been added, as the extension protocol and type have no functionality without extensions.

@cnoon cnoon self-requested a review Mar 26, 2019

@cnoon cnoon self-assigned this Mar 26, 2019

@cnoon cnoon added this to the 5.0.0-beta.4 milestone Mar 26, 2019

@cnoon
Copy link
Member

left a comment

Overall the change looks great @jshier, just a few minor comments. I think it's important to be consistent throughout the codebase with this change. We should probably go all in or not at all IMO.

Show resolved Hide resolved Source/ServerTrustEvaluation.swift
Show resolved Hide resolved Source/Session.swift
Show resolved Hide resolved Source/URLRequest+Alamofire.swift

@cnoon cnoon assigned jshier and unassigned cnoon Mar 26, 2019

@jshier

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

@cnoon Back to you. If you disagree about which extensions should be inside the extension point, let me know and I can move everything inside.

@cnoon
Copy link
Member

left a comment

One more thing that I have questions on the second time around. Sorry I missed this the first time.

Show resolved Hide resolved Source/AlamofireExtended.swift
Show resolved Hide resolved Source/AlamofireExtended.swift
@cnoon

cnoon approved these changes Mar 27, 2019

Copy link
Member

left a comment

Looks good @jshier! Thanks for putting up with me learning along the way with this PR. 馃槈

@cnoon cnoon merged commit 34b43bd into master Mar 27, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@cnoon cnoon deleted the feature/extension-namespace branch Mar 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.