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

feat: improved error UX #15

Merged
merged 1 commit into from
Oct 13, 2022
Merged

feat: improved error UX #15

merged 1 commit into from
Oct 13, 2022

Conversation

pgautier404
Copy link
Collaborator

Update error handing to conform to client SDK spec. Uses isXXX() methods such as asHit, asError, etc. to provide type-safe instances and informative IDE code completion.

Copy link
Contributor

@cprice404 cprice404 left a comment

Choose a reason for hiding this comment

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

looks great, just one minor question to consider. it's something we can punt until later though if we don't come to an immediate consensus.

{
return $this;
}
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

question: did you consider the tradeoffs between null vs throwing an exception here?

On the one hand, if someone calls this incorrectly, if we throw an exception then we can give a nice clear message about what went wrong, vs. the null pointer exception that they are likely to get otherwise.

On the other hand, doing it this way probably allows people to write code like this if they want to:

if ($foo = client.Get('blah', 'blah')) {
  // handle happy path
} else {
  // handle sad path
}

without any try/catch shenanigans.

I don't want to hold up your progress so we should definitely timebox any consideration on this and just keep moving, but floating it because I expect that python/JS/PHP are going to end up looking pretty similar for this stuff since they don't have algebraic data types.

Copy link
Contributor

Choose a reason for hiding this comment

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

@malandis I'm curious if you have any strong opinions on this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My thinking was that we have made a contract with the user that our cache client will always return and not throw exceptions. I would be upset as a user if I had to jump through a relatively exotic syntax and wrap the whole thing in a try/catch. As you pointed out, this also allows a pseudo-pattern matching scheme using assignments in if statements. The null pointer exception should be pretty easy to diagnose since the return type signature of these methods is explicitly "response class or null". We'll obviously want to demonstrate the patterns we end up deeming best practices in the docs.

Copy link
Contributor

Choose a reason for hiding this comment

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

That seems pretty compelling. Thanks for laying it out. 🚢

@pgautier404 pgautier404 merged commit 8979539 into main Oct 13, 2022
@malandis malandis deleted the convert-error-responses branch November 1, 2024 21:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants