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

Adding support for NSCopying protocol. #200

Merged
merged 3 commits into from
Nov 9, 2017
Merged

Conversation

JThramer
Copy link
Contributor

@JThramer JThramer commented Nov 8, 2017

What it says on the box. Making RouteObject conform to NSCopying. Needed for mapbox/mapbox-navigation-ios#806.

/cc @1ec5 @bsudekum

@bsudekum bsudekum requested a review from 1ec5 November 8, 2017 22:28
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.

Since the set of properties on RouteOptions keeps expanding, please add unit tests so we can be sure everything copies over correctly.

/**
Returns a copy of the object.
- parameter zone: an NSZone to use in the copy operation.
- returns: A copy of the object.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this method already inherits documentation from the protocol’s method declaration, at least as far as Quick Help and other tools are concerned.

@frederoni
Copy link
Contributor

Would it be possible to avoid all this boilerplate code by adopting Codable now that we're on an Xcode 9 stack already?

@JThramer
Copy link
Contributor Author

JThramer commented Nov 9, 2017

I agree @frederoni, but wouldn't it be better to do that refactor in one tech-debt chore, rather than piecemeal? There are 8 classes that use NSSecureCoding, and two that use NSCopying, so it seems like there's enough of a payload to justify a chore here.

}

@objc(isEqualToRouteOptions:)
open func isEqual(to routeOptions: RouteOptions?) -> Bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, this gets us a little bit closer to implementing #151. 👍

@JThramer JThramer merged commit 9a1c7e3 into master Nov 9, 2017
@JThramer JThramer deleted the jerrad/routeobject-copying branch November 9, 2017 18:51
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.

3 participants