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

Add support for Swift Package Manager #362

Merged
merged 25 commits into from
Jun 11, 2019
Merged

Add support for Swift Package Manager #362

merged 25 commits into from
Jun 11, 2019

Conversation

frederoni
Copy link
Contributor

@frederoni frederoni commented Mar 23, 2019

Fixes 318 - Support for static library using CocoaPods
Fixes #234 - Swift Package Manager on macOS

  • Audit Obj-C bridgeability
  • Deprecate MapboxDirections.swift on CocoaPods public repo in favor of MapboxDirections (Reverted pod rename to minimize the changes)

cc @1ec5

@frederoni frederoni force-pushed the fred/spm branch 4 times, most recently from fc1dea3 to 7833e8a Compare March 23, 2019 02:37
@1ec5
Copy link
Contributor

1ec5 commented Mar 27, 2019

I wonder if we can replace all extensible and bitmask Obj-C enums. 🤔

Unfortunately, that won’t be feasible as long as this library supports Objective-C. Would it be possible to maintain parallel types, with CocoaPods and Carthage using the Objective-C version and SPM using the Swift version?

@frederoni
Copy link
Contributor Author

Would it be possible to maintain parallel types

Great idea. That should be relatively straightforward, and only four duplicated data types in the shim, which shouldn't be too inconvenient to maintain.

@frederoni frederoni force-pushed the fred/spm branch 2 times, most recently from be1bc31 to f2515a3 Compare April 4, 2019 13:35
Package.swift Outdated
],
dependencies: [
// Dependencies declare other packages that this package depends on.
.package(url: "../Polyline", .branch("fred/spm4.2"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Remember to update this line before merging.

@frederoni frederoni force-pushed the fred/spm branch 2 times, most recently from 0321f46 to 1d59309 Compare April 9, 2019 13:09

/**
Initializes a new `Lane` using the given lane indications.
*/
#if SWIFT_PACKAGE
Copy link
Contributor Author

@frederoni frederoni Apr 9, 2019

Choose a reason for hiding this comment

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

I'm not entirely satisfied with this workaround, having to provide duplicated initializers.
What I would like to see is a pre-processor macro:

#if SWIFT_PACKAGE
#define OBJC_NO_SPM
#define OBJC_NO_SPM(x)
#else
#define OBJC_NO_SPM @objc
#define OBJC_NO_SPM(x) @objc(x)
#endif

but an Obj-C macro would not work with SPM and SPM doesn't support these macros.

@objcMembers solves roughly half of the issues, since it ignores the @objc-flag if it's not able to bridge properly to Obj-C, but it only works for some methods and properties.

@frederoni
Copy link
Contributor Author

OHHTTPStubs doesn't provide support for SPM so we're not running tests on the SPM flavor yet.

@frederoni frederoni force-pushed the fred/spm branch 2 times, most recently from 3c9181d to 3bc1d47 Compare April 15, 2019 10:41
Package.swift Outdated
],
dependencies: [
// Dependencies declare other packages that this package depends on.
.package(url: "https://github.com/raphaelmor/Polyline.git", .branch("master"))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@1ec5 Would you mind making a new release of Polyline?

Changes since v4.2.0:
Added support for Swift Package Manager (#51)

Copy link
Contributor

Choose a reason for hiding this comment

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

@frederoni frederoni force-pushed the fred/spm branch 3 times, most recently from 6b7a031 to 176c4bb Compare April 15, 2019 12:15
/**
A component that represents a lane representation of an instruction.
*/
@objc(MBLaneIndicationComponent)

This comment was marked as resolved.

@frederoni frederoni marked this pull request as ready for review April 16, 2019 11:17
@1ec5 1ec5 added the build label Apr 24, 2019
@@ -219,6 +221,7 @@ public enum InstructionFormat: UInt, CustomStringConvertible {

You do not create instances of this class directly. Instead, create instances of `MatchOptions` or `RouteOptions`.
*/
@objcMembers
Copy link
Contributor

Choose a reason for hiding this comment

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

Huh, didn’t know about this annotation. Good find. There are still some bare @objc keywords in the class; are they redundant now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@objc is still needed for refinements. It's also useful for compile-time errors of unbridgeable types.
Only using @objcMembers would fail silently if a type all of a sudden doesn't bridge to Objective-C. Haven't figured out a best practice yet, but as long as we have Obj-C bridging tests, a missing @objc annotation is fine since it would get caught by the test at compile-time.

@@ -382,7 +390,12 @@ open class DirectionsOptions: NSObject, NSSecureCoding, NSCopying {

This property should be set to `MBDirectionsProfileIdentifierAutomobile`, `MBDirectionsProfileIdentifierAutomobileAvoidingTraffic`, `MBDirectionsProfileIdentifierCycling`, or `MBDirectionsProfileIdentifierWalking`. The default value of this property is `MBDirectionsProfileIdentifierAutomobile`, which specifies driving directions.
*/
#if SWIFT_PACKAGE
open var profileIdentifier: MBDirectionsProfileIdentifier
#else
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this conditional compilation still necessary now that the overall class is annotated with @objcMembers?

Copy link
Contributor Author

@frederoni frederoni Jun 3, 2019

Choose a reason for hiding this comment

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

good catch, not needed.

@frederoni
Copy link
Contributor Author

We can put Obj-C++ inside a separate compat dependency in SPM and compile the Objective-C code along with the Swift framework and therefore support Objective-C w/o having to maintain duplicated types for the SPM version.

@frederoni frederoni force-pushed the fred/spm branch 2 times, most recently from 6c22ddd to 2265f23 Compare June 7, 2019 21:36
@@ -418,7 +425,7 @@ open class DirectionsOptions: NSObject, NSSecureCoding, NSCopying {

By default, no attribute options are specified. It is recommended that `routeShapeResolution` be set to `.full`.
*/
@objc open var attributeOptions: AttributeOptions = []
open var attributeOptions: MBAttributeOptions = []
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the type remain as AttributeOptions, or does it have to be downgraded to MBAttributeOptions?

@1ec5 1ec5 changed the title Rename to MapboxDirections and add support for Swift Package Manager Add support for Swift Package Manager Jun 11, 2019
Copy link
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

Suggested changelog entry:

  • Added support for Swift Package Manager. (#362)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Swift Package Manager
2 participants