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.

Sign up for GitHub

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

Jump to bottom

Bridge profile identifier to Objective-C #106

Merged
merged 3 commits into from
Feb 20, 2017
Merged

Bridge profile identifier to Objective-C #106

merged 3 commits into from
Feb 20, 2017

Conversation

frederoni
Copy link
Contributor

Global variables defined in Swift doesn't bridge to Objective-C.

@frederoni frederoni requested a review from 1ec5 January 17, 2017 20:07
@frederoni frederoni self-assigned this Jan 17, 2017
1ec5
1ec5 suggested changes Jan 17, 2017
View reviewed changes
MapboxDirections/MBRouteOptions.h Outdated

This profile prioritizes fast routes by preferring high-speed roads like highways. A driving route may use a ferry where necessary.
*/
extern NSString *const MBDirectionsProfileIdentifierAutomobile;
Copy link
Contributor

Choose a reason for hiding this comment

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

While you’re at it, can you make these constants into an extensible string enumeration?

@frederoni frederoni force-pushed the fred-objc-compatibility branch 2 times, most recently from 0827ecc to bcab052 Compare January 18, 2017 12:22
@frederoni frederoni changed the base branch from master to swift3 January 18, 2017 12:33
@frederoni frederoni force-pushed the fred-objc-compatibility branch 2 times, most recently from 26d2fdc to 77670cb Compare January 18, 2017 12:55
@frederoni frederoni mentioned this pull request Jan 18, 2017
Closed
@frederoni
Copy link
Contributor Author

Rebased onto the swift3 branch.

@1ec5
Copy link
Contributor

1ec5 commented Jan 18, 2017

Actually, for consistency, it'd be better to rebase atop master and merge into swift3, as we've done for all the other changes.

1ec5
1ec5 reviewed Jan 18, 2017
View reviewed changes
MapboxDirections/MBRouteOptions.h Outdated

#pragma mark - Specifying the Routing Profile

typedef NSString * MBDirectionsProfileIdentifier NS_EXTENSIBLE_STRING_ENUM;
Copy link
Contributor

Choose a reason for hiding this comment

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

This symbol could use some documentation.

MapboxDirections/MBRouteOptions.swift Outdated
self.profileIdentifier = profileIdentifier ?? MBDirectionsProfileIdentifierAutomobile
self.allowsUTurnAtWaypoint = ![MBDirectionsProfileIdentifierAutomobile, MBDirectionsProfileIdentifierAutomobileAvoidingTraffic].contains(self.profileIdentifier)
self.profileIdentifier = profileIdentifier ?? MBDirectionsProfileIdentifier.automobile.rawValue
self.allowsUTurnAtWaypoint = ![MBDirectionsProfileIdentifier.automobile.rawValue, MBDirectionsProfileIdentifier.automobileAvoidingTraffic.rawValue].contains(self.profileIdentifier)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should profileIdentifier be of type MBDirectionsProfileIdentifier?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a breaking change and it involves quite a few changes but it would make things clearer.

Copy link
Contributor

Choose a reason for hiding this comment

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

This library hasn't reached 1.0 yet, so breaking changes are perfectly fine.

@1ec5 1ec5 mentioned this pull request Jan 19, 2017
Closed
@frederoni frederoni force-pushed the fred-objc-compatibility branch 2 times, most recently from 4c977ea to f2fc96d Compare January 19, 2017 15:19
@frederoni frederoni changed the base branch from swift3 to master January 19, 2017 16:16
@frederoni frederoni changed the base branch from master to swift3 January 19, 2017 16:18
@frederoni
Copy link
Contributor Author

frederoni commented Jan 20, 2017 •
edited
Loading

Modified the workflows to only run carthage if there's a Cartfile and removed the excessive Carthage integration workflows. This solved the bitrise issues for tvOS, macOS & iOS but OHHTTPStubs and Polyline don't have a watchOS target so we have to fix it upstream or disable polylines for watchOS.

@frederoni frederoni force-pushed the fred-objc-compatibility branch 3 times, most recently from 46a68ef to d8fe15a Compare January 20, 2017 12:58
@1ec5
Copy link
Contributor

1ec5 commented Jan 20, 2017

OHHTTPStubs and Polyline don't have a watchOS target

Polyline does have a watchOS target, but it looks like a watchOS scheme was omitted – probably an oversight.

@1ec5 1ec5 changed the base branch from swift3 to master February 20, 2017 05:57
@1ec5 1ec5 merged commit c15cdb4 into master Feb 20, 2017
@1ec5 1ec5 deleted the fred-objc-compatibility branch February 20, 2017 06:22
@1ec5 1ec5 added the backwards incompatible changes that break backwards compatibility of public API label Dec 18, 2018
@1ec5 1ec5 added this to the v0.8.0 milestone Dec 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@1ec5 1ec5 1ec5 approved these changes

Assignees

@frederoni frederoni

Labels
backwards incompatible changes that break backwards compatibility of public API objective-c
Projects
None yet
Milestone
v0.8.0
Development

Successfully merging this pull request may close these issues.
2 participants
@frederoni @1ec5