refactor: Upgrade api to match latest dart conventions

[jtdLab]

@ra1u Thanks for this great package.

Are there any plans of improving the api to match latest dart conventions e.g. improving function names, and better usage of the dart type system. Maybe i can help :)

[ra1u]

This sound great!

Exception vs Error issue is currently addressed by @exaby73 in PR #77 .

For this PR it seem that we have breaking change that requires major release. It would be nice if we get full list of changes that we would like to push for next major release.

@jtdLab can you provide list of changes you would like to address, or open issue for each. Then we can all contribute for next major release.

[erf]

Hi, i would also like to see some changes, i'll try to list them here:

1. The _connection should have the specific type given in the comment.

class Command {
  /*RedisConnection*/ var _connection;

This is a much safer way and we know how the rest of the code should behave.

2. non null types

class RedisConnection {
  Socket? _socket;
  LazyStream? _stream;

Here you could make this non null by using a builder pattern i mentioned in https://github.com/ra1u/redis-dart/pull/42/files

This way you also avoid having to force (!) potential null values

3. format the code using dart format . (i just made a PR)

4. replace then with await where it makes sense

5. replace get_connection with an getter get connection

6. bump sdk version ?

7. _senddummy what is going on here? is this needed?

8. improve comments use first cap

9. improve names _sendraw -> _sendRaw

10. simplify the API (too many connect methods? remove Command.from) ?

[ra1u]

Thanks for feedback.

Hi, i would also like to see some changes, i'll try to list them here:

1. The _connection should have the specific type given in the comment.

class Command {
  /*RedisConnection*/ var _connection;

This is a much safer way and we know how the rest of the code should behave.

I agree. Last time I tried to fix this there was some typing issue here. IIRC due to having incorrect type/class stack among classes sharing this type.

2. non null types

class RedisConnection {
  Socket? _socket;
  LazyStream? _stream;

Here you could make this non null by using a builder pattern i mentioned in https://github.com/ra1u/redis-dart/pull/42/files

This way you also avoid having to force (!) potential null values

I agree,

3. format the code using dart format . (i just made a PR)

Merged

4. replace then with await where it makes sense

Last time we tested this, await had performance issue.
PR in his direction should have:

1. performance reports
2. in case of performance regression, justification on why this is acceptable.

5. replace get_connection with an getter get connection

That is fine. Should we keep backward compatibility?

6. bump sdk version ?

What version and why?

7. _senddummy what is going on here? is this needed?

_senddummy is used when we dont want to send anything, but we expect processing underlying Future.
Idea behind is, that library is build around assertion that for every 1 send, we have exactly 1 receive.
When there is no send, but is expected receive we send dummy data.
Why? Because this 1/1 idea allows us to send more than one package, before receiving one package and then do correct dispatching to awaiter.
Relevant discussion: #93
Fell free to fix/improve this, but take this into account. It might be that I am not aware of more idiomatic way to implement this goals.

8. improve comments use first cap

Agree. Should we care about backward compatibility?

9. improve names _sendraw -> _sendRaw

Same as before at 8.

10. simplify the API (too many connect methods? remove Command.from) ?

Can you post what you have in mind? To many classes, simpler api? Please post code that you have have in mind from user perspective. (how this affects examples we have in README)

---

I am glad that we addressed this issues as most of them were also on my radar. My goal is for improvements is in a way that will allow us to continuously improve and support codebase with users in mind.

Fell free to send pull requests addressing each issue individually (or more in case of similar issues)

[erf]

Thanks for your feedback. I'm glad you are open to improve on these areas. I can try to add some PR's on these when i have time. I can try to address some of your comments.

1-3. Good
4. if you do await then it will finish that task before continue, if doing multiple then after each others they will do that work in parallel, so need to see what makes sense here
5. i think get connection is better and i wouldn't worry about breaking changes, people can just update their code. Also if we introduce all these changes it would be a major version bump
6. if we bump Dart SDK version to => 3 < 4 we would support all the new features introduced there like patterns, records, switch expressions etc. i would use the latter in the parsing code (which also could need some love)
7. i believe this could be simplified but haven't really looked into it much yet
8. backward compatibility for comments?
9. don't need backward compatibility for private methods
10. i would have to look closer at the API but as you say in the README "simple by design", i only really need to connect to redis and do a send_object.

BTW. I looked at the PubSub class and it looks a bit .. chaotic .. then, while, try nested loops, forcing null .. and also the _stream inside the RedisConnection makes me think there are several code patterns that could be improved upon, and maybe it would be better to remove some functionality (PubSub?) and start with a simpler foundation. I'm not sure how much of changes you'd like to do or how much time you plan to work on this, and i also have other projects i'd like to work on and i know there are alternatives like the resp_client although not updated in a while, looks way better, so not sure how much time i should be spending on this.

[erf]

What is going on here?

  Transaction(Command command) : super(command._connection) {
    _overrided_command = command;
    //we override his _connection, during transaction
    //it is best to point out where problem is
    command._connection = _WarningConnection();
  }

You're trying to set the arbitrary class _WarningConnection to the connection which just knows how to throw an exception. (Not a RedisConnection)

[ra1u]

About PubSub spaghetti:

Just like whole dart app can run concurrently over single thread, we can run redis client concurrently over single socket.

This (lets run everything over single socket) works as long as there is 1/1 correspondence (one tx message for one rx message). PubSub breaks this, because socket can receive message from redis at any time. To prevent this we inject error on connection that was hijacked by PubSub for this reason.

---

Same should apply for transactions.

However it seems It would probably be better if we would change transaction interface into await interface.

Take note that it is not possible to do

t = await conn.multi();
i = await t.send_object("incr");
j = await t.exec();

taking into account i is returned (with response from redis). Because results of each command is available only after t.exec() is executed.

For this reason we would need to return list of responses from all previous responses as j, and i would represent just dummy Future, that gets awaited as long it takes to send data over the socket. This makes implementation much simpler.

---

We started this project before async/await was available in dart.
Hope that this clears up. Do not hesitate to ask further questions.

[erf]

I think it's perhaps better that you, as the author, does some initial refactoring. I'm not really familiar with the code base as is, neither with Redis.

[ra1u]

@erf Thaks for your input.

Can you put some motivations we are trying to achive here, so that we can justify updates and changes to our users?

Do we need more reliable code?
Do we need support for recent sdk, version?
Do we have specific bugs to fix?
Do we miss some features?

I would rather move towards aciveing some goals, rather than just saying lets fix this code because it does not look great.

[erf]

I think the changes i've mentioned will make the code more robust and clean. When i look at the state of the code now, it's hard to wrap my head around what's going on so therefor i feel less comfortable using the package.

[erf]

Do we need more reliable code?

I think you DO want as reliable and clean code as possible when potentially many people are using your package for critical stuff. If you think it's reliable enough, then leave it as is, but the issues i mentioned i think should be addressed.

Do we need support for recent sdk, version?
Do we have specific bugs to fix?
Do we miss some features?

These are not as relevant for this issue, but if you want to update to support to the latest SDK with the latest language features then why not.