search

ably / ably-ruby

Ruby client library SDK for Ably realtime messaging service
https://ably.com/download
Apache License 2.0
36 stars 19 forks source link

[ECO-4687]Feature/resume recover #402

Closed sacOO7 closed 4 months ago

sacOO7 commented 5 months ago
lawrence-forooghian commented 4 months ago

There might be failing tests on the PR. Those will be addressed as a part of (…)

We can't merge a PR with failing tests into main since it will stop us from being able to do a release. I suggest that you create an integration branch e.g. integration/protocol-v2 and make this PR target that branch (once #391 is merged).

lawrence-forooghian commented 4 months ago

I was wondering if you'd seen https://github.com/ably/engineering/blob/33da110db02504e329109fd4618eee76d79dd0ec/best-practices/commits.md, which explains some best practice for Git commit / history structure? I think that some of the guidance there might be helpful here.

sacOO7 commented 4 months ago

There might be failing tests on the PR. Those will be addressed as a part of (…)

We can't merge a PR with failing tests into main since it will stop us from being able to do a release. I suggest that you create an integration branch e.g. integration/protocol-v2 and make this PR target that branch (once #391 is merged).

Yes, I have already created a branch feature/no-connection-serial. All of the no-connection-serial related PR's will be merged into it. We will merge feature/no-connection-serial into main only after making sure all of the tests passes. About fix/tests, I was planning to merge it into feature/no-connection-serial, but we can also merge it into main once all review comments are addressed.

lawrence-forooghian commented 4 months ago

About fix/tests, I was planning to merge it into feature/no-connection-serial, but we can also merge it into main once all review comments are addressed.

Yeah, I agree that that one should go into main without waiting for all of the no connection serial work to be merged.

sacOO7 commented 4 months ago

@sacOO7 I am not sure how to review this PR. There's no overall description to explain in detail what changes it aims to make (e.g. which spec points it aims to implement).

So I thought I’d try reviewing it commit-by-commit, hoping that the commit messages would give me useful information. But immediately I found that the commits haven't been structured with reviewability in mind. For example, 1d0f959 implements a class without any tests, and then the next commit (07c053f) fixes some code from the previous commit (i.e. meaning that someone reviewing commit-by-commit has to look at the initial attempt, spend some time getting confused by it because it's wrong, and then eventually realise that it gets fixed in the next commit), and they spend time getting confused about why the commit that introduces the class has no tests, only to again realise that they get introduced in the next commit, and they spend time wondering why there is no test for what happens when the recovery key can't be decoded, only to eventually realise that for some reason this test is added about 10 commits later (d654665).

And I’d argue that given that this PR contains a mixture of features and refactors and typo fixes, it needs to be reviewable commit-by-commit; there’s too much noise for the PR to be reviewable simply by looking at the overall diff.

Do you have any suggestions for how I can review this PR in its current state? If not, please could you put it back into draft and try to make it more reviewable?

I agree we don't have atomic commits. This partially also has to do with a fact that current implementation is split across codebase for a given atomic spec ( also pointed out in the comment -> https://github.com/ably/ably-ruby/issues/407#issuecomment-2129397204 ). I will try to add more description for changes being implemented at the top of the PR. Also, will try to add more meaningful commits going forward. For now, I would request to the review the PR code changes. Also, at the end, we will have a final working PR against main with full no-connection-serial implementation, so the code itself will make sense and there won't be a need to look at the commits.

lawrence-forooghian commented 4 months ago

This partially also has to do with a fact that current implementation is split across codebase for a given atomic spec

That has nothing to do with the example I gave above though (a feature which is implemented in a broken fashion without tests, then some tests are added and the code is fixed, then later on another test that should have been there in the first place is added). That's just a matter of making sure that once you've finished your work, you take some time to fix the commit history.

lawrence-forooghian commented 4 months ago

I will try to add more description for changes being implemented at the top of the PR. Also, will try to add more meaningful commits going forward. For now, I would request to the review the PR code changes.

I will attempt to review this PR. But please think about reviewability in your future PRs. It would make my life as a reviewer a lot easier.

lawrence-forooghian commented 4 months ago

Also, at the end, we will have a final working PR against main with full no-connection-serial implementation, so the code itself will make sense and there won't be a need to look at the commits.

Sure, but I still need to be able to review the PRs.

lawrence-forooghian commented 4 months ago

And, as stated in the linked commit guidance, "the history is the deliverable, not the code".

sacOO7 commented 4 months ago

@lawrence-forooghian I have created a separate PR with proper commit history -> #409 I have tried to keep changes atomic and code easy to understand. I will also add PR summary, so it will be easy to review the code. That said, I will close this PR and discussion can be continued on #409