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

Tweaks to DecodingFailureInitializable #71

Merged
merged 11 commits into from
Oct 5, 2018
Merged

Tweaks to DecodingFailureInitializable #71

merged 11 commits into from
Oct 5, 2018

Conversation

wmcginty
Copy link
Contributor

  • Adds the target type information when a decoding failure occurs
  • Cleans up some duplicate code to use the RequestDefault functionality
  • Deprecates the 'seat of pants' choose a root decoding key at run time functionality.

@codecov-io
Copy link

codecov-io commented Sep 21, 2018 •
edited
Loading

Codecov Report

Merging #71 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #71      +/-   ##
==========================================
+ Coverage   96.62%   96.64%   +0.01%     
==========================================
  Files          33       36       +3     
  Lines        1185     1192       +7     
==========================================
+ Hits         1145     1152       +7     
  Misses         40       40
Impacted Files Coverage Δ
Sources/Hyperspace/HTTP/HTTP.swift 100% <ø> (ø) ⬆️
Tests/DecodingTests.swift 95.39% <ø> (-0.09%) ⬇️
...perspace/Request/Decoding/DecodableContainer.swift 100% <100%> (ø) ⬆️
Sources/Hyperspace/Request/Request.swift 96.07% <100%> (-0.65%) ⬇️
Tests/DecodingFailureTests.swift 100% <100%> (ø)
Tests/Helper/Mocks/MockBackendService.swift 42.85% <100%> (ø) ⬆️
...urces/Hyperspace/Extensions/AnyError+Request.swift 100% <100%> (ø)
Tests/Helper/XCTestCase+JSON.swift 100% <100%> (ø)
... and 1 more

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 1a80c49...fe99e42. Read the comment docs.

@wmcginty
Copy link
Contributor Author

I'm not sure how to feel about the Codebeat issue. The similar code it's flagging is calling two entirely different codepaths, even if the words are similar.

tylermilner
tylermilner suggested changes Sep 24, 2018
View reviewed changes
Tests/DecodingFailureTests.swift Outdated

// MARK: - Helper

private func loadedJSONData(fromFileNamed name: String) -> Data {
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like we've already got this helper method in DecodingTests.swift. Can you go ahead and factor it out so that we don't have the same code in 2 spots?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can do. Will probably add it as an extension to XCTestCase.

tylermilner
tylermilner previously approved these changes Sep 24, 2018
View reviewed changes
@wmcginty
Copy link
Contributor Author

@tylermilner Any strong opinions about the Codebeat issue? Is it against the spirit of using it to turn off the rule?

@tylermilner
Copy link
Contributor

@wmcginty I kind of feel like it is. I know it's caught a code duplication issue in the past that we fixed (#19) so I would prefer to leave it on, even if it flags some false positives every once in a while. I'm not sure if there's a way we can "ignore" the current issue though to allow for merging. Last I looked, it seemed like Codebeat could use some maturity in managing the rules that it uses to scan your code.

tylermilner
tylermilner previously approved these changes Oct 3, 2018
View reviewed changes
@wmcginty
Copy link
Contributor Author

wmcginty commented Oct 3, 2018 •
edited
Loading

@tylermilner Added a changelog entry

tylermilner
tylermilner previously approved these changes Oct 3, 2018
View reviewed changes
@wmcginty
Copy link
Contributor Author

wmcginty commented Oct 5, 2018

Fixed some merge conflicts. Hopefully Codebeat can finish it's check now.

tylermilner
tylermilner suggested changes Oct 5, 2018
View reviewed changes
CHANGELOG.md
@@ -5,6 +5,9 @@
* Remove the type definitions deprecated in 2.0.0
[Will McGinty](https://github.com/wmcginty)
[#72](https://github.com/BottleRocketStudios/iOS-Hyperspace/pull/72)
* Added failing type information to `DecodingFailureInitializable` allowing the API to make decisions based off of the type that failed to decode and deprecate dynamically keyed decoding.
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor, but can we get a newline before this entry?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Stupid merge.

@tylermilner tylermilner merged commit df588a6 into master Oct 5, 2018
@tylermilner tylermilner deleted the feature/errors branch October 5, 2018 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tylermilner tylermilner tylermilner approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@wmcginty @codecov-io @tylermilner