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

improve error handling in ui module #1870

Merged
merged 2 commits into from
Oct 28, 2022
Merged

Conversation

naosense
Copy link
Contributor

Replacing unwrap in ui module with error propagation.

Related to #1753

@naosense naosense temporarily deployed to cachix October 28, 2022 08:36 Inactive
@naosense
Copy link
Contributor Author

Hi @har7an , could you please review this pr?

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.

Thanks for your continued help with the error handling!

I've left a few small remarks on your code. Overall this looks good and it helps us a lot.

Comment on lines +248 to +255
.fatal()
.cmp(
&self
.z_indices
.iter()
.position(|id| id == *b_id)
.with_context(err_context)
.fatal(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you using .fatal() instead of ? here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because returning result is not allowed in a closure

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I missed the closure above, my bad!

zellij-server/src/panes/plugin_pane.rs Outdated Show resolved Hide resolved
zellij-server/src/panes/terminal_pane.rs Outdated Show resolved Hide resolved
zellij-server/src/panes/tiled_panes/mod.rs Outdated Show resolved Hide resolved
zellij-server/src/ui/boundaries.rs Outdated Show resolved Hide resolved
zellij-server/src/ui/overlay/mod.rs Outdated Show resolved Hide resolved
zellij-server/src/ui/overlay/prompt.rs Outdated Show resolved Hide resolved
zellij-server/src/ui/pane_boundaries_frame.rs Outdated Show resolved Hide resolved
zellij-server/src/ui/pane_boundaries_frame.rs Outdated Show resolved Hide resolved
zellij-server/src/ui/pane_boundaries_frame.rs Outdated Show resolved Hide resolved
@har7an
Copy link
Contributor

har7an commented Oct 28, 2022

And on a sidenote, it seems you beat me to the change in ui/boundaries by a few minutes: #1871. :-)

But that's okay, since your PR touches many more things I'll close mine and we'll go with yours instead.

@naosense naosense temporarily deployed to cachix October 28, 2022 10:07 Inactive
@naosense
Copy link
Contributor Author

Thank you for your patient review, I learned a lot from your comments

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 contribution! And thank you for being so open to my remarks.

@har7an har7an merged commit c5b1eb0 into zellij-org:main Oct 28, 2022
har7an added a commit that referenced this pull request Oct 28, 2022
Remove calls to `unwrap` in `zellij_server::ui::*`
@naosense naosense deleted the err_handling branch October 30, 2022 11:49
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.

2 participants