ra1u / redis-dart

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

Refactor RedisConnection #42

Closed erf closed 3 years ago

erf commented 3 years ago

Refactor RedisConnection (Breaking change!)

Let connection take a socket in the constructor so it's always non-null and final, and we don't have to force it, which could lead to null-errors.

We also make _stream non-null and final, by initializing it in the constructor as well.

Also make the connect methods into static builder methods for creating and returning a RedisConnection instead of a Command. The user will now just pass this to a new Command. This seem more logic to me as a RedisConnection don't have to know about Command.

ra1u commented 3 years ago

Hi! I think that now user can make multiple Commands from single connection. This will likely brake something or am I missing something.

erf commented 3 years ago

Ok, i can fix it to return a Command.

But now you could still make multiple commands from a connection, or? Perhaps we should make the constructor private? But not sure how everything works now..

ra1u commented 3 years ago

Idea is that it should be restricted to make only one Command per RedisConnection. That is what we currently have.

ra1u commented 3 years ago

About how it works.

User constructs RedisConnection, but insted of RedisConnection it gets Command. With standard constructors of RedisConnection you would get RedisConnection, but here you get Future\<Command>.

One example is Future<Command> connect(host, port)

I hope that make more understandable, let me know if you did not understand something.

erf commented 3 years ago

I now made the RedisConnection.connect static methods return a Command (as before). I made the RedisConnection constructor private, so it only can be instantiated via the connect method.

I removed the parser member as i could not see it being used anywere (!)

I made the Command.get_connection to a connection getter.

Not sure i get the meaning of the future in the _senddummy / getdummy method.

Also don't know why Command._connection is not a RedisCommand type, but a var, and why you set it to _WarrningPubSubInProgress or _WarningConnection.

Also why does send_nothing return a optional value?

ra1u commented 3 years ago

I think we should make constructor of Command private but available for RedisConnection instead of making private constructor of RedisConnection . Currently user can still make multiple Commands from Command.get_connection / connection.

WarrningPubSubInProgress is used to invalidate current Command. This way during PubSub we throw exception if user uses wrong Command. We send exception if any Command get serialized trough RedisConnection except what runs in PubSub class. Because of this type conversions, we keep _connection in Command as generic. We also have tests for this in test/pubsub_test.dart

Let me know if you find better and simple implementation for this feature as I believe, that current implementation can be improved.

erf commented 3 years ago

The changes i've made now, is mostly for cleaning up and making the code more secure, and does not change much from before, except that now the connect methods are now static factory methods for creating a command with a connection.

I thing this is a good first change, but i rather not touch much more (per now at least), as the there is some of the code i don't really understand and seem a bit odd to me.

erf commented 3 years ago

Ok, i've made one more commit which makes the Command constructor private.