LGFae
commented
5 months ago The codebase has indeed become rather unruly. I've tried refactoring it a bit to make it easier to parse, but I was already planning another refactor to make this clearer in general.
That said, there are a few things I would like to point out about this:
1- first, I would really rather not increase our dependencies, specially for the daemon. Every dependency increases compilation time, specially for people downstream, since they will have to download them from crates.io. Also, from what I've seen of anyhow in this PR, it doesn't appear to make things that much better, it just changes some map_err(format!(...)) into a single function call.
2 - there will be a lot more nitpicking for this in general, in comparison to other PRs. I would strongly recommend maybe dividing this into multiple PRs, if possible, so that both me and you can have an easier time writing/reviewing/communicating in general. For example, I believe the code reorganization into 3 subcrates could be its own PR. Then, just the Socket refactor could be another. Improvements to the documentation could be another, etc.
3 - be ready for more nitpicks than usual. I can already see some stuff I will probably be asking you to change. For example, please remove pedantic from the used clippy lints. That won't really help us much, and will greatly pollute rust-analyzer's output with things that don't matter.
Overall, since I was planning on doing this anyway, it would be a welcome improvement. But I do think it would be better to separate it into different PRs so we could review it one step at a time. I've made some gigantic PRs lately, but they were often changing a lot of the code's internal logic all at once, so it was hard to compartmentalize them. It seems to me that this PR, on the other hand, could be divided in smaller chunks. This would also have the benefit that, should you try changing something I don't like, we can just ignore that PR and still use all the others.
EDIT: if your problem is understanding the codebase, might I recommend writing documentation for the functions instead? That would still be immensely helpful, would make you understand everything better, would make it possibly easier to do later refactors like what you are thinking, and would make the codebase easier to understand. Though it is much less exciting than a large refactor.
rkuklik
Hello,
I wanted to learn from this codebase and all too often got stuck when reading the code. I would like to refactor it to (what I consider) more idiomatic Rust. My goal was to organize IPC and friends around a single data structure with associated functions instead of passing file descriptors around and then to look into the large memory usage. I don't want to adjust behaviour (yet).
@LGFae would you be OK with this effort? I think it would be fairly major refactor, with preview shown in these first four commits.
Thank you very much for your time.