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.

Sign up for GitHub

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

Jump to bottom

improve error handling in route #1808

Merged
merged 5 commits into from
Oct 21, 2022
Merged

improve error handling in route #1808

merged 5 commits into from
Oct 21, 2022

Conversation

naosense
Copy link
Contributor

Replacing unwrap in route.rs with error propagation.

Related to #1753

@har7an har7an mentioned this pull request Oct 18, 2022
Open
@har7an
Copy link
Contributor

har7an commented Oct 18, 2022

Hi again and thanks for working on this!

Are you interested in expanding this PR a bit and helping us to remove all calls to unwrap from route.rs? In particular I think it would be great if you change route_thread_main to return a Result<()> and then call .fatal() here. Then we can remove route from the list of files to improve error handling in for good. What do you say?

@naosense naosense temporarily deployed to cachix October 18, 2022 14:17 Inactive
@naosense
Copy link
Contributor Author

Thanks for your detailed explanation, it's a pleasure to do it

@naosense
Copy link
Contributor Author

hi @har7an , could you please review my code again?

@naosense naosense temporarily deployed to cachix October 20, 2022 08:57 Inactive
har7an
har7an requested changes Oct 20, 2022
View reviewed changes
Copy link
Contributor

@har7an har7an left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for continuing to work on this!

It seems last time I looked over your code I was a bit sloppy, and I want to apologize for that. If you apply the changes below this is good to go.

Edit: And you'll have to rebase your code, I see someone changed route.rs on main in the meantime.

zellij-server/src/route.rs Show resolved Hide resolved
zellij-server/src/lib.rs Show resolved Hide resolved
@naosense naosense force-pushed the route_error_handling branch from 2edeeeb to 86510a3 Compare October 21, 2022 03:07
@naosense naosense requested a review from har7an October 21, 2022 03:10
@naosense naosense temporarily deployed to cachix October 21, 2022 13:15 Inactive
har7an
har7an approved these changes Oct 21, 2022
View reviewed changes
Copy link
Contributor

@har7an har7an left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done, thanks for your effort!

zellij-server/src/route.rs
let mut retry_queue = vec![];
let err_context = || format!("failed to handle instruction for client {client_id}");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thank you!

@har7an har7an added the hacktoberfest-accepted PRs counting towards hacktoberfest label Oct 21, 2022
@har7an har7an merged commit e5115e8 into zellij-org:main Oct 21, 2022
har7an added a commit that referenced this pull request Oct 21, 2022
@har7an
changelog: Add PR #1808
d36e91e 
Improve error handling in `server/route`.
@naosense naosense deleted the route_error_handling branch October 21, 2022 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@har7an har7an har7an approved these changes

Assignees
No one assigned
Labels
hacktoberfest-accepted PRs counting towards hacktoberfest
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@naosense @har7an