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

Please refactor your unwrapping optionals #228

Closed
AF-cgi opened this issue Jan 8, 2018 · 9 comments · Fixed by #382
Closed

Please refactor your unwrapping optionals #228

AF-cgi opened this issue Jan 8, 2018 · 9 comments · Fixed by #382

Comments

@AF-cgi
Copy link

AF-cgi commented Jan 8, 2018

I use MapboxDirections 0.15.1. After response my crashed with an unwrapping optional issue in MBRouteStep line 600.
let drivingSide = DrivingSide(description: json["driving_side"] as! String) ?? .right
I resolve the issue by:
let drivingSide = DrivingSide(description: json["driving_side"] as? String ?? "") ?? .right
I can imaging that they are many other optional issues in the project.

@bsudekum
Copy link

bsudekum commented Jan 8, 2018

Thanks for the bug report @AFcgi, this sounds reasonable to me.

@bsudekum
Copy link

bsudekum commented Jan 8, 2018

Actually, after thinking about this more, driving_side is always present on a RouteStep when making a request to Mapbox Directions API. @AFcgi are you hitting our API or have you stood up your own server?

@AF-cgi
Copy link
Author

AF-cgi commented Jan 9, 2018

Hi @bsudekum,
thanks for your reply. We use an own server. The driving_side isn't present on our RouteStep. Also we use SwiftLint. One of these rules says do not unwrapped optionals in your code. So I think it's a better way to unwrap all your optionals.

@1ec5
Copy link
Contributor

1ec5 commented Jan 9, 2018

Related: mapbox/MapboxGeocoder.swift#106.

We should replace most of the force-unwrapping and assertions with exceptions, reserving assertions for things that are entirely within the control of the client-side library (and not due to a malformed response or an error in the application code).

@bsudekum
Copy link

@1ec5 asserting would not really help @AFcgi out here, correct?

@1ec5
Copy link
Contributor

1ec5 commented Jan 17, 2018

To clarify:

  • When dealing with input from the developer or from the Directions API, throw exceptions.
  • Otherwise, when dealing with data internal to the library, assert or force-unwrap (preferably the former).

In general, this means that initializers would be failable and non-initializers would have more assertions.

@1ec5
Copy link
Contributor

1ec5 commented Jan 18, 2018

In general, this means that initializers would be failable and non-initializers would have more assertions.

Migrating to JSONDecoder entails making the initializers failable. #221 still force-unwraps some things, but it should be much easier to throw instead now.

/cc @frederoni

@1ec5
Copy link
Contributor

1ec5 commented Nov 2, 2019

#382 adopts Codable and eliminates nearly all assertions and upfront optional unwrapping.

@1ec5
Copy link
Contributor

1ec5 commented Nov 21, 2019

#382 also added a preconditions to the coding methods that are enforced by force-unwrapping optionals. #388 makes the preconditions gentler by throwing errors instead, but that change should probably go in a separate PR.

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 a pull request may close this issue.

3 participants