Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor of IPC and error handling #329

Closed
wants to merge 4 commits into from
Closed

Conversation

rkuklik
Copy link
Contributor

@rkuklik rkuklik commented Jun 18, 2024

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.

@LGFae
Copy link
Owner

LGFae commented Jun 18, 2024

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
Copy link
Contributor Author

rkuklik commented Jun 18, 2024

Ok, I'll try to split this into multiple PRs. I wanted anyhow for better backtraces (when chaining errors) and ? working out of the box.

Should I close this PR and start sending few small ones, given that this is reaching hundreds of lines already?

@LGFae
Copy link
Owner

LGFae commented Jun 18, 2024

Should I close this PR and start sending few small ones, given that this is reaching hundreds of lines already?

I think that would be ideal, yeah.

I wanted anyhow for better backtraces (when chaining errors) and ? working out of the box.

You can use anyhow for the client, if it makes backtraces better. But note that we don't really want to expose backtraces to the end user. I would rather just give them an easy to understand error ("failed to decode png"). I don't know how easy to parse (as a human) anyhow's backtraces are, or if we can configure its behavior for release builds...

@rkuklik
Copy link
Contributor Author

rkuklik commented Jun 18, 2024

Alright, be right back.

@rkuklik rkuklik closed this Jun 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants