callum-ryan commented on PR #503: URL: https://github.com/apache/iceberg-rust/pull/503#issuecomment-2266670947
> Hi @callum-ryan 👋 > > I can help with reviewing this PR since I recently completed the [Memory Catalog](https://github.com/apache/iceberg-rust/pull/475) implementation for iceberg-rust. I took a quick look at your PR and I could leave a lot of comments but I worry that would be a little demotivating. Instead, I want to see if we can try something a little different. > > I have commented 6 suggestions below. I recommend you accept each of these suggestions blindly (just click on the `Commit suggestion` button next to each one). These suggestions add ~1500 lines of code defining 59 tests that cover all the behaviours I would expect from a correctly functioning `Catalog` implementation. Currently, your implementation passes 25 of them. Here's the fun part: Try refactoring your code so it passes the remaining 34 tests as well before we continue with the review (let me know if you get stuck, I'm happy to help!) > > Does that sounds like a good plan to you? 👍 👎 Thanks for this @fqaiser94, I like this approach! I will work on the refactor most likely tomorrow -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For additional commands, e-mail: issues-h...@iceberg.apache.org
