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

Adopt Codable and JSONDecoder #221

Closed
wants to merge 27 commits into from
Closed

Adopt Codable and JSONDecoder #221

wants to merge 27 commits into from

Conversation

frederoni
Copy link
Contributor

@frederoni frederoni commented Dec 7, 2017 •
edited
Loading

Fixes #134

  • Codable
  • JSONDecoder
  • Fix outletIndexes
  • Fix visual instruction imageBaseURL
  • Re-enable route options tests

@bsudekum @1ec5 👀

@frederoni frederoni self-assigned this Dec 7, 2017
@frederoni frederoni added the improvement Improvement for an existing feature. label Dec 7, 2017
1ec5
1ec5 approved these changes Dec 7, 2017
View reviewed changes
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.

Yay!

Is there anything we can do to make this extension fit in more naturally?

@frederoni frederoni force-pushed the fred-refactor-codable branch 4 times, most recently from 1c7c808 to fa0f5f3 Compare December 13, 2017 15:02
@frederoni
Copy link
Contributor Author

frederoni commented Dec 13, 2017 •
edited
Loading

Is there anything we can do to make this extension fit in more naturally?

Replaced the extension with an UncertainCodable<Geometry, String>.

@frederoni
Copy link
Contributor Author

V4 compatibility was pretty isolated before. I wonder if I made that part worse. 🤔

@frederoni frederoni force-pushed the fred-refactor-codable branch 2 times, most recently from 48550ab to 5dfc04c Compare December 15, 2017 16:10
@JThramer JThramer mentioned this pull request Dec 18, 2017
Closed
JThramer
JThramer approved these changes Dec 18, 2017
View reviewed changes
Copy link
Contributor

@JThramer JThramer left a comment

Choose a reason for hiding this comment

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

:shipit:

@bsudekum
Copy link

Before merging, let's double check this integrates properly with another library that uses this as a dependency.

@frederoni frederoni mentioned this pull request Dec 19, 2017
Closed
1ec5
1ec5 reviewed Dec 19, 2017
View reviewed changes
MapboxDirections/MBDirections.swift Outdated
errorHandler(apiError)
}
guard let data = data, response?.mimeType == "application/json" else {
errorHandler(Directions.informativeError(describing: [:], response: response, underlyingError: error as NSError?))
Copy link
Contributor

Choose a reason for hiding this comment

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

This call used to take a JSON object whose code and message keys determined the message. Does that no longer matter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Errors are handled more gracefully in 1a161d6

@frederoni
Copy link
Contributor Author

frederoni commented Dec 20, 2017 •
edited
Loading

I haven't noticed any changes or regressions in mapbox-navigation-ios after these changes but let's hold off a bit more. V5Tests.testGeoJSON frequently times out on CI with a timeout set to 2 seconds.

Before Codable, testGeoJSON consistently finished at 750ms compared to 1.7s after adopting Codable. It's a 3.2MB JSON but still.

@frederoni
Copy link
Contributor Author

Managed to get it down to 1.3s but we may want to wait for https://bugs.swift.org/browse/SR-6252

@frederoni frederoni mentioned this pull request Jan 18, 2018
Merged
@1ec5 1ec5 mentioned this pull request Jan 18, 2018
Closed
@bsudekum bsudekum mentioned this pull request Feb 16, 2018
Merged
@1ec5 1ec5 mentioned this pull request Mar 13, 2018
Merged
@frederoni frederoni mentioned this pull request Mar 20, 2018
Merged
@frederoni
Copy link
Contributor Author

The bottleneck persists in Swift 4.2

@1ec5 1ec5 mentioned this pull request Feb 12, 2019
Closed
JThramer pushed a commit that referenced this pull request Sep 27, 2019
Finishing up. Only remaining thing to do is replace NSSecureCoding …
d2d54fc 
…behavior with `Codable`, which is in-flight in #221.
@JThramer JThramer mentioned this pull request Sep 27, 2019
Merged
1 task
1ec5 pushed a commit that referenced this pull request Oct 30, 2019
@1ec5
Finishing up. Only remaining thing to do is replace NSSecureCoding …
bf4e249 
…behavior with `Codable`, which is in-flight in #221.
1ec5 pushed a commit that referenced this pull request Nov 2, 2019
@1ec5
Finishing up. Only remaining thing to do is replace NSSecureCoding …
a4a78e7 
…behavior with `Codable`, which is in-flight in #221.
@1ec5
Copy link
Contributor

1ec5 commented Nov 23, 2019

Superseded by #382.

@1ec5 1ec5 closed this Nov 23, 2019
@1ec5 1ec5 mentioned this pull request Dec 21, 2019
Closed
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

@JThramer JThramer JThramer approved these changes

Assignees

@frederoni frederoni

Labels
⚠️ DO NOT MERGE improvement Improvement for an existing feature.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Adopt JSONDecoder
4 participants
@frederoni @bsudekum @1ec5 @JThramer