search

y-crdt / ydotnet

.NET bindings for yrs.
MIT License
29 stars 9 forks source link

Improvements to the library #55

Closed SebastianStehle closed 9 months ago

SebastianStehle commented 9 months ago

As discussed, I have created a new PR. I have documented the changes in a separate file: https://github.com/SebastianStehle/ydotnet/blob/for-pr/docs/decision.md

LSViana commented 9 months ago

Hi, @SebastianStehle!

I'm pausing the review for today but I'll continue it later.

Also, I believe you can keep updating the for-pr branch with your changes in the main branch of your repository, this way this branch will not lag out while the review is going on.

SebastianStehle commented 9 months ago

After our meeting I decided to remove clustering for now and add it later as a beta package or so.

LSViana commented 9 months ago

Hi, @SebastianStehle!

I didn't have time to check this yesterday or today, I'm sorry. I intend to verify it tomorrow.

LSViana commented 9 months ago

Additionally, some unit tests aren't passing right now. If you have the time to check them, it'd be good. Otherwise, I can do it and fix them later.

LSViana commented 9 months ago

@SebastianStehle Will you have some time to check the comments I posted here?

SebastianStehle commented 9 months ago

Done. I am.on mobile, so I hope I do not miss anything. Haven't had a look to the tests yet, but I noticed that the length of the random client ID has a big impact on all the length assertions.

SebastianStehle commented 9 months ago

I have added a few more comments for stuff that I have not seen before.

LSViana commented 9 months ago

Done. I am.on mobile, so I hope I do not miss anything. Haven't had a look to the tests yet, but I noticed that the length of the random client ID has a big impact on all the length assertions.

I'm not sure what you mean about the client ID. The current implementation is using 53-bit numbers as you suggested before.

Is there something to be changed in the ClientIdGenerator class?

LSViana commented 9 months ago

@SebastianStehle I think we've handled most of the conversations here and I applied some code changes to reduce duplication and complexity in the core layer, as I wanted originally.

I'm approving the PR already and I'll just try to fix the unit tests before I merge it. But I'll probably have to do it tomorrow since I'm out of time for this today.

SebastianStehle commented 9 months ago

I have just merged in my latest fixes to the server project. It is exclusively for the server and extensions and used in production. So nothing to review here.

Can we also disable 1600 again. It spills out warnings when XML comments are missing for internal members and the build process produces warnings for public members anyway, so it does not provide any value.

For me SA1201 and SA1202 are also annoying, because ...

  1. If you change one member from protected to private you have to move it and cause much more changes than needed.
  2. Some methods just belong together, e.g. if public method A calls private method B then they should be close together.

SA 1003 and 1009 (closing parenthesis) probably conflict with your editor settings. I would not have a space here with my settings, but you make them. It is fine for me, but we should then adjust the stylecop setting.

return (TransactionUpdateResult) TransactionChannel.ApplyV2(Handle, stateDiff, (uint) stateDiff.Length);
goldsam commented 8 months ago

Wow. Did you really take my work on the build pipeline and not credit me at all? i spent quite a bit of time on that.

LSViana commented 8 months ago

Hi, @goldsam!

Wait, I'm sorry if that happened. I merged Sebastian's .yml files into the repository and I didn't really pay attention to where they came from.

So, just to clarify, do you mean the pipeline setup that was added by Sebastian or me?

I can most definitely add you as one of the authors. I just wasn't aware of it before so I'm sorry for putting you in the position of having to ask for that.

goldsam commented 8 months ago

Just the pipeline setup. I spent a lot of time and trial and error getting it working to support a wide range of platforms. Just disappointed that I didn't get at least a little credit 😢

SebastianStehle commented 8 months ago

This was not intended. I wanted to have something quick for my PR, so I copied from you what I understood and made some things differently, what I did not understand. and because a wide range of reasons the PR became too big. I am not sure if there are some big differences or if I have made something worse.

LSViana commented 8 months ago

@goldsam @SebastianStehle I added the authors as a comment to all the .yml GitHub workflow files.

goldsam commented 7 months ago

I really appreciate you trying to make this right. That was very considerate of you. 😉