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

Conform MoyaError to CustomNSError in addition to LocalizedError #1783

Merged
merged 5 commits into from Mar 1, 2019

Conversation

Projects
None yet
6 participants
@dpoggi
Copy link
Contributor

commented Dec 20, 2018

Since the existing version of MoyaError conforms to LocalizedError, its cases with associated (underlying) Swift.Error values have no means of returning that information from userInfo[NSUnderlyingErrorKey], where it can be seen by crash reporting tools that speak NSError.

Motivation

My frustration, looking at a run-of-the-mill "handled" error in Bugsnag, when I realized I couldn't determine the key path at which Codable's object mapping had failed.

In this PR branch, MoyaError conforms to CustomNSError in addition to LocalizedError so it can supply values for both NSLocalizedDescriptionKey and NSUnderlyingErrorKey.

Implementation

  • The error domain (String(reflecting: MoyaError.self)), codes (Swift's internal ordinal value for each enum case), and localized descriptions stay the same.
  • Even though it's returned as part of the user info dictionary now, the localized description is still available as someMoyaError.localizedDescription.
  • Tests have been added for MoyaError's bridged user info dictionary. They ensure that NSLocalizedDescriptionKey and the Swift localizedDescription property return the same string, and that the appropriate enum cases return an underlying error for NSUnderlyingErrorKey.

P.S.

I recognize that an unsolicited PR is less than ideal, and I do apologize. This is something I thought I ought to do while I had the spare minutes and the inspiration.

If this gets closed, so be it — "the people have spoken." Just thought I'd offer, since it's not a whole ton of code, and I don't think I'm the only person whose time it could save.

@MoyaBot

This comment has been minimized.

Copy link

commented Dec 20, 2018

1 Warning
⚠️ Consider adding supporting documentation to this change. Documentation can be found in the docs directory.
3 Messages
📖 iOS: Executed 276 tests, with 0 failures (0 unexpected) in 11.742 (11.891) seconds
📖 tvOS: Executed 276 tests, with 0 failures (0 unexpected) in 10.989 (11.131) seconds
📖 macOS: Executed 276 tests, with 0 failures (0 unexpected) in 12.203 (12.319) seconds

Generated by 🚫 Danger

@SD10

This comment has been minimized.

Copy link
Member

commented Dec 20, 2018

Hey @dpoggi

I recognize that an unsolicited PR is less than ideal, and I do apologize. This is something I thought I ought to do while I had the spare minutes and the inspiration.

Always recommend opening an issue but nonetheless thanks for the PR 👍

My frustration, looking at a run-of-the-mill "handled" error in Bugsnag, when I realized I couldn't determine the key path at which Codable's object mapping had failed.

I'm definitely a little skeptical about modifying Moya for better integration with other libraries that use NSError, one of our principles is to provide a Swift-first API/abstraction for making network requests. But I can understand your frustration 😞

If I'm reading this right (I'm not familiar with CustomNSError), it looks like the errorDescription property would be deprecated with this change, so it's API breaking.

@Moya/contributors do you have any input here?

@dpoggi

This comment has been minimized.

Copy link
Contributor Author

commented Dec 20, 2018

As far as I'm aware, there's nothing in the way of conforming both protocols to maintain API compatibility. I can revert to the original declaration (with Swift.Error) as well in case any tooling is sensitive to it.

EDIT: Branch has been updated.

@dpoggi dpoggi force-pushed the dpoggi:enhancement/customnserror branch from 566b93f to 216fa10 Dec 20, 2018

@dpoggi dpoggi changed the title Conform MoyaError to CustomNSError instead of LocalizedError Conform MoyaError to CustomNSError in addition to LocalizedError Dec 20, 2018

@AndrewSB
Copy link
Member

left a comment

Overall, I think we should probably add this. I don't see the downside of adding better support for NSErrors. If this broke linux support, I'd feel differently.

I think it would be better if this wasn't a breaking change, just an additive one though.

Show resolved Hide resolved Sources/Moya/MoyaError.swift Outdated
@sunshinejr

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

Hey @dpoggi! Will you be able to finish the work on this one? I think it would be a valuable contribution to Moya 13.

@dpoggi

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2019

Happy to pick this back up. I'll be in touch this evening.

@sunshinejr

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

@dpoggi awesome, thanks!

@dpoggi dpoggi force-pushed the dpoggi:enhancement/customnserror branch 3 times, most recently from da8fdbc to fd8bfb0 Feb 27, 2019

@dpoggi

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2019

Updated with a solution that accomplishes what the PR originally set out to do with a smaller diff and less convoluted code.

API-wise, to the best of my knowledge, this is purely additive. If there's anything now that anyone feels should have comments with links to the docs on error bridging, let me know, I've got 'em on deck.

@dpoggi dpoggi force-pushed the dpoggi:enhancement/customnserror branch from fd8bfb0 to c44bebc Feb 27, 2019

@codecov-io

This comment has been minimized.

Copy link

commented Feb 27, 2019

Codecov Report

Merging #1783 into development will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##           development    #1783   +/-   ##
============================================
  Coverage        90.83%   90.83%           
============================================
  Files                5        5           
  Lines              131      131           
============================================
  Hits               119      119           
  Misses              12       12

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c9b516e...dd7ee94. Read the comment docs.

@pedrovereza pedrovereza requested a review from AndrewSB Feb 27, 2019

@AndrewSB
Copy link
Member

left a comment

i'm a fan, great job.

i think this is mergeable, but i'll wait to let someone else merge just in case i've missed something

@sunshinejr
Copy link
Member

left a comment

This is nice, thanks again @dpoggi! Left you one comment, other than that it should be good to go from the code perspective.

Could you also please change the base branch to develop + add a changelog entry? 😉

Show resolved Hide resolved Sources/Moya/MoyaError.swift Outdated

dpoggi added some commits Dec 19, 2018

@dpoggi dpoggi changed the base branch from master to development Feb 28, 2019

@dpoggi dpoggi force-pushed the dpoggi:enhancement/customnserror branch 2 times, most recently from a40935d to 5e75bd4 Feb 28, 2019

@dpoggi dpoggi force-pushed the dpoggi:enhancement/customnserror branch from 5e75bd4 to dd7ee94 Feb 28, 2019

@dpoggi

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2019

@sunshinejr should be all set. Thanks for the feedback! 😄

@sunshinejr

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

@dpoggi thanks so much for taking care of that!

@sunshinejr sunshinejr merged commit 4125728 into Moya:development Mar 1, 2019

2 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: carthage_without_swiftlint_integration Your tests passed on CircleCI!
Details
@peril-moya

This comment has been minimized.

Copy link

commented Mar 1, 2019

@dpoggi Thanks a lot for contributing to Moya! We've invited you to join
the Moya GitHub organization – no pressure to accept! If you'd like more
information on what that means, check out our contributor guidelines and
feel free to reach out to @Moya/core-team with any questions.

Generated by 🚫 dangerJS

@dpoggi dpoggi deleted the dpoggi:enhancement/customnserror branch Mar 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.