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

Replaced generic struct based AWSAppSyncClientError by a typed enum… #35

Merged
merged 5 commits into from
Nov 14, 2018
Merged

Replaced generic struct based AWSAppSyncClientError by a typed enum… #35

merged 5 commits into from
Nov 14, 2018

Conversation

MarioBajr
Copy link
Contributor

The struct based AWSAppSyncClientError doesn't provide enough informations about the error. Requiring the user to do string matching with additionalInfo to figure out what exactly went wrong.

Description of changes:

  • Replaced the struct based AWSAppSyncClientError by a typed enum based, providing better description for each type of error, including inner errors when applied.
  • Moved common request code into a single method in AWSAppSyncHTTPNetworkTransport.

Considerations:

  • Good to have in mind that this PR changes the public interface. Being non backward compatible with written code.

@MarioBajr MarioBajr changed the title - Replaced generic struct based AWSAppSyncClientError by a typed enum… Replaced generic struct based AWSAppSyncClientError by a typed enum… Jun 7, 2018
@rohandubal
Copy link
Contributor

Hello @MarioBajr

Thanks for the PR!

As you have noted, this would be a breaking change, I was wondering if we could make the struct a property of AWSAppSyncClientError instead of completely replacing the type. Any particular reason you think we need to completely replace this?

Thanks,
Rohan

@MarioBajr
Copy link
Contributor Author

Sure thing, gonna replace tomorrow to the enum be a property of AWSAppSyncClientError.
Cheers

@rohandubal
Copy link
Contributor

Awesome! thanks @MarioBajr will wait for the update!

@MarioBajr
Copy link
Contributor Author

Hi @rohandubal. How do you feel about the additions of the redundant parameters with deprecated flag as an effort not to break the backward compatibility, but keeping the interface clean in the long run?

@available(*, deprecated, message: "use the enum pattern matching instead")
public var body: Data? {
    switch self {
    case .parseError(let data, _, _):
        return data
    case .requestFailed(let data, _, _):
        return data
    case .noData:
        return nil
    }
}

@available(*, deprecated, message: "use the enum pattern matching instead")
public var response: HTTPURLResponse? {
    switch self {
    case .parseError(_, let response, _):
        return response as? HTTPURLResponse
    case .requestFailed(_, let response, _):
        return response as? HTTPURLResponse
    case .noData:
        return nil
    }
}

@available(*, deprecated)
var isInternalError: Bool {
    return false
}

@available(*, deprecated)
var additionalInfo: String? {
    switch self {
    case .parseError(_, _, _):
        return "Could not parse response data."
    case .requestFailed(_, _, _):
        return "Did not receive a successful HTTP code."
    case .noData:
        return "No Data received in response."
    }
}

@rohandubal
Copy link
Contributor

I think that is a fair approach. I will discuss it with the team and get back to you if we are all clear for this approach. Thanks again!

@rohandubal rohandubal self-requested a review June 28, 2018 23:05
@rohandubal rohandubal self-assigned this Jun 28, 2018
@rohandubal
Copy link
Contributor

Reviewing the latest changes.

@rohandubal
Copy link
Contributor

Hello @MarioBajr

The changes look great! However, I am unable to merge due to conflicts. Is it possible for you to rebase onto master? I will release them as soon as they are rebased.

Thanks,
Rohan

@MarioBajr
Copy link
Contributor Author

Done 👍

@rohandubal
Copy link
Contributor

Awesome, thanks @MarioBajr Will merge it soon 😄

Mario Araujo added 5 commits September 29, 2018 21:51
@palpatim palpatim assigned palpatim and unassigned rohandubal Nov 13, 2018
@palpatim palpatim merged commit d7c6ff4 into awslabs:master Nov 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution Community contribution PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants