mattpolzin / JSONAPI

Swift Codable JSON:API framework
MIT License
75 stars 19 forks source link

Better error message reporting for includes and add localizedDescription #85

Closed mlomeli closed 4 years ago

mlomeli commented 4 years ago

This is the code I wrote, for https://github.com/mattpolzin/JSONAPI/issues/84

About the contributing Guidelines, I don't know how to test it as the playground didn't run for me, but I ran it in my project and it worked. So feel free to disregard the pull request. Just wanted to see how to share the code in a readable way.

I added the LocalizedError protocol, so you print the description when you use the localizedDescription property of the error.

mattpolzin commented 4 years ago

Thanks for opening a PR with an implementation of a better error reporting experience! I will dig into this tonight and see how it compares in a few situations where I tend to see this type of error pop up.

You will need to modify and/or add to the unit tests for the package. When you push changes, the tests run automatically, so I can see from this PR that the following test is now failing https://github.com/mattpolzin/JSONAPI/blob/ebd11df104d4e9bb8b0faf7c8cdb985ce831d7d9/Tests/JSONAPITests/Document/DocumentDecodingErrorTests.swift#L98. It makes sense for that test to start failing since your code modified the desired output from similar errors, but the test needs to be rewritten to assert the new desired output is found so no one else accidentally changes it.

I don't know how to test it...

The easiest way to go about writing and testing the library is generally to open it in Xcode where Product -> Test in the menu (Cmd + U is the keyboard shortcut) will run all the same tests that the CI check runs when you submit the PR.

Alternatively, you can browse to the directory containing the library from a shell and run swift test --enable-test-discovery.

I added the LocalizedError protocol

This is a nice thought, thank you! However, this library happens to be able to operate entirely without the Foundation library (you will notice there are no other import Foundation lines (except in test files) and I would like to keep it that way. Another way to put that is this library is purely Swift code and can be compiled on any platform that has a Swift compiler, which is an even broader set of platforms than necessarily have the Foundation library available.

mattpolzin commented 4 years ago

Taking a closer look at the test failure, it states:


Include Resource2 failed to parse because of found JSON:API type "not_an_author" but expected "authors"

is not equal to

Include 3 failed to parse: Could not have been Include Type 1 because: found JSON:API type "not_an_author" but expected "articles"

Could not have been Include Type 2 because: found JSON:API type "not_an_author" but expected "authors"


The first message in there is the new message as of this PR. The second message is the old message prior to this PR (for the failure that particular test case sets up).

What I am noticing is that the message printed by the code in this PR does not contain as much information. The old message indicates two things:

  1. The index of the include that failed to parse (because the includes array can contain any number of includes so we need to know which one was malformed).
  2. For each possible type of include, a reason why the include at the given index could not be parsed as that particular type.
mattpolzin commented 4 years ago

I think I see what you were trying to accomplish here. I think it was confusing how the old error message stated the index of the failed include in one place and the index of the failed type in another place. Does that sound like what about the old message was confusing? In other words, the old message had two numbers and it seemed like they both referred to the same thing, so you simplified the message by just grabbing the thing being referred to.

mlomeli commented 4 years ago

Yeah, I see that you added it in https://github.com/mattpolzin/JSONAPI/pull/86 which addresses the issue.