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

fix(envs): Unify operation of Zellij environment variables #842

Merged
merged 1 commit into from
Nov 10, 2021
Merged

fix(envs): Unify operation of Zellij environment variables #842

merged 1 commit into from
Nov 10, 2021

Conversation

ken-matsui
Copy link
Contributor

This PR aims to prevent Zellij environment variable manipulation from being scattered in various places of code and to make adding new variables such as ZELLIJ_PANE_ID easy.

Copy link
Contributor

@a-kenji a-kenji left a comment

Choose a reason for hiding this comment

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

LGTM in general, I have one small question though!

@@ -0,0 +1,24 @@
/// Uniformly operates ZELLIJ* environment variables
use anyhow::Result;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need the anyhow::Result 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.

@a-kenji
Ah, I just thought it would be preferable to unify and absorb all result types to anyhow::Result that would be clear as I did on #824 (comment).

If you enact how to handle result types as shown below, I think anyhow::Result should be removed from this PR:

  1. Use anyhow::Result if a function returns various error types like feat(attach): Support --index option for attach sub-command to choose the session indexed by provided number in the active sessions ordered creation date #824 (comment)
  2. If a function returns only one error type, just propagate it

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for the insight,
I think the list you suggested would be preferable,
unless there is a clear benefit to using anyhow::Result everywhere.

What do you think?

Copy link
Contributor Author

@ken-matsui ken-matsui Nov 10, 2021

Choose a reason for hiding this comment

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

@a-kenji

Additionally, the main function also can return anyhow::Result as shown here.

As I mentioned this here, we can have the main function return anyhow::Result. This means all functions can perfectly back forward errors to main and a parent process with almost panicless using ? operator. I personally think this is the benefit to use anyhow::Result everywhere. However, this takes a lot of work, and the benefit may not be worth the effort; in addition, some situations might need intentionally panicking.

To conclude, I think putting anyhow::Result everywhere will be preferable when the following conditions are satisfied:

  1. If you want to collect all errors into the main function
  2. If you want to avoid almost all panics, unwrap, and process::exit

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, my bad I thought it would still work that way with a normal Result.
But it explicitly needs to return the anyhow::Result.
Thank you for that explanation again!

@a-kenji a-kenji merged commit 6d60d83 into zellij-org:main Nov 10, 2021
@ken-matsui ken-matsui deleted the unify-operation-of-zellij-environment-variables branch November 10, 2021 08:09
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