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

Swifty APIs #176

Merged
merged 5 commits into from
Sep 13, 2017
Merged

Swifty APIs #176

merged 5 commits into from
Sep 13, 2017

Conversation

edjiang
Copy link
Contributor

@edjiang edjiang commented Aug 31, 2017

Update public APIs to adhere to the Swift API Design Guidelines.

Most of the changes in this PR are minor, and are related to renaming APIs, and renaming the consumer of the API in the tests.

The primary structural change in this PR is to change the Configuration from a static object to a singleton, which will allow us in the future to improve testability and pave the way for multiple Uber Clients, if necessary for the end developer.

@edjiang edjiang changed the title Swift APIs Swifty APIs Aug 31, 2017
@edjiang edjiang requested a review from bobz August 31, 2017 22:05
@edjiang edjiang mentioned this pull request Sep 1, 2017
7 tasks
@edjiang edjiang changed the base branch from remove-china-support to develop September 6, 2017 18:30
@@ -107,6 +107,7 @@ import MapKit

}

// TODO: fix
Copy link

Choose a reason for hiding this comment

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

Let's not to commit TODOs.

Copy link
Contributor Author

@edjiang edjiang Sep 12, 2017

Choose a reason for hiding this comment

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

Done.

@@ -96,6 +96,7 @@ import UIKit

/**
* Object representing an access scope to the Uber API
* TODO: investigate why this is here
Copy link

Choose a reason for hiding this comment

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

Let's not commit TODOs. Link an issue if there's followup work to do.

Copy link
Contributor Author

@edjiang edjiang Sep 12, 2017

Choose a reason for hiding this comment

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

Done.

@@ -87,48 +84,98 @@ import WebKit
*/
@objc(UBSDKConfiguration) open class Configuration : NSObject {
// MARK : Variables
open static var shared: Configuration = Configuration()
Copy link

Choose a reason for hiding this comment

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

Doesn't this put us back in the Singleton situation? Is this a stopgap, or what's intention here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed in person, this leaves us in the singleton situation but opens the possibility to injecting Configuration() objects into the RidesClient in the future to support multiple profiles.

@edjiang edjiang merged commit ffddf05 into develop Sep 13, 2017
@edjiang edjiang deleted the swifty-apis-pt-1 branch September 15, 2017 18:25
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.

2 participants