search

BottleRocketStudios / iOS-Hyperspace

An extremely lightweight wrapper around URLSession to make working with APIs a breeze.
Apache License 2.0
47 stars 17 forks source link

Changed AnyError's implementation of NetworkServiceFailureInitializable #95

Closed rickbdotcom closed 5 years ago

rickbdotcom commented 5 years ago

To allow access to underlying failureResponse.

CLAassistant commented 5 years ago

CLA assistant check
All committers have signed the CLA.

wmcginty commented 5 years ago

Thanks for the contribution @rickbdotcom ! The changes look good to me - can you fix the resulting broken unit tests and add a changelog entry before merging?

rickbdotcom commented 5 years ago

Will do!

codecov-io commented 5 years ago

Codecov Report

Merging #95 into master will increase coverage by <.01%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #95      +/-   ##
==========================================
+ Coverage   94.61%   94.62%   +<.01%     
==========================================
  Files          46       46              
  Lines        1542     1543       +1     
==========================================
+ Hits         1459     1460       +1     
  Misses         83       83
Impacted Files Coverage Δ
Tests/NetworkServiceTests.swift 100% <100%> (ø) :arrow_up:
Sources/Hyperspace/Request/AnyError.swift 57.14% <100%> (ø) :arrow_up:
Tests/RecoverableTests.swift 94.93% <100%> (ø) :arrow_up:
Tests/BackendServiceTests.swift 98.64% <100%> (ø) :arrow_up:

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 7742f7d...b95235e. Read the comment docs.

wmcginty commented 5 years ago

@rickbdotcom This looks great, if you can just add an entry to Changelog.md I think we're good to go!

rickbdotcom commented 5 years ago

OK updated change log. Now one issue I can see with this change is that it could break some code if a consumer of the Framework was depending on the underlying error being NetworkServiceError and doing a cast of AnyError.error to NetworkServiceError.

wmcginty commented 5 years ago

@rickbdotcom I don't think that's a huge issue assuming we bump the minor version number and call the impact of the change out. Thoughts @tylermilner ?

tylermilner commented 5 years ago

@wmcginty sounds good to me. @rickbdotcom looks like there's a conflict in CHANGELOG.md. Could you resolve so that we can merge?