search

ra1u / redis-dart

fast redis protocol parser and client
MIT License
84 stars 35 forks source link

fix: make errors into idiomatic exceptions #77

Open exaby73 opened 1 year ago

exaby73 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 10 months ago

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 commented 9 months ago

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