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

fix: make errors into idiomatic exceptions #77

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

exaby73
Copy link

@exaby73 exaby73 commented Jul 21, 2023

This PR aims to make the exceptions in this library more idiomatic with Dart conventions.

This also helps solve linting errors with stronger lint configurations such as ones found on the lint package

@ra1u
Copy link
Owner

ra1u commented Jul 21, 2023

Looks good.
Is this backward compatible or do we need to bump major version before release?

Can you share what lintier was used. We welcome having linted code. Maybe we should extend CI/CD so that it fails if linter is not happy.

@exaby73
Copy link
Author

exaby73 commented Jul 21, 2023

I believe lint has very strict lints enabled. This is backward compatible right now with the *Error classes having deprecation notices instead of removing them :) You can decide when you want a breaking change. I was thinking of also getting a PR out extending the min Dart SDK version to be to 3. Since that would be a breaking change, maybe you could remove the deprecated errors in that release

@ra1u
Copy link
Owner

ra1u commented Jul 21, 2023

I understand now why Exception is much is better than Error, but I am reserved against breaking changes. (nobody likes to change code due to backward compatibility).

Can you volunteer few test that shows that code is backward compatible?

@exaby73
Copy link
Author

exaby73 commented Jul 21, 2023

Sure. Since the *Error classes extend the new *Exception classes, it should be backward compatible. I'll see what I can come up with in tests to prove this sometime tomorrow. Thanks for your support on this, and also your work on it :)

@exaby73
Copy link
Author

exaby73 commented Jul 22, 2023

Ahh I rechecked this. Sorry I was wrong. This is not backwards compatible. Throwing a RedisExeption for example and catching a RedisError will not work, since RedisError extends RedisException and not the other way around. You can see this by checking out my branch and running the error_test.dart file and making isRedisError a TypeMatcher<RedisError>() instead of TypeMatcher<RedisException>() which it is in my branch.

I will try to see if I can come up with a solution to this, but in the case this is not possible to make backwards compatible without writing some less than ideal code, would this be a deal breaker?

@ra1u
Copy link
Owner

ra1u commented Dec 22, 2023

Hey. First thanks for your effort.
I have take look into this and I got shower thought.

What you have done here is
class RedisError extends RedisException

But if we do other way around:
class RedisException extends RedisError

We can now throw or return RedisException in our code and existing code expecting to catch/receive RedisError should still work.

Am I correct?

@exaby73
Copy link
Author

exaby73 commented Jan 1, 2024

Hey @ra1u. I've updated the PR based on your feedback. Let me know if there is anything else I need to do :)

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