Load config before start() #423
Conversation
|
Thank you, this is awesome!
This is just a small thing.
This is quite an awesome side benefit of this change! Now even parts of changes are possible to test reasonably well in the integration tests. Good job!
Also a good idea!
This was a translation error, thanks for catching that. Just write a message if you get that test fixed and feel its ready for a proper review, then I'll have another look at it. |
|
Thanks for your quick review in spite of such a large diff 🙏 One of the tests always fails on GitHub Actions. I'm looking into this, but I'm not sure why it happens... |
| } | ||
| } | ||
|
|
||
| let config_dir = opts.config_dir.clone().or_else(install::default_config_dir); |
There was a problem hiding this comment.
I misunderstood the behavior of default_config_dir, which I thought would create a directory.
default_config_dir just indicates a possible configuration directory, so it should be called here as the previous implementation.
By the way, as default_config_dir returns Option<PathBuf>, I made opts.config_dir call clone() to match these types. Maybe, I can delete this clone() by refactoring default_config_dir, but it should be done in another PR.
There was a problem hiding this comment.
I misunderstood the behavior of
default_config_dir, which I thought would create a directory.
default_config_dirjust indicates a possible configuration directory, so it should be called here as the previous implementation.
Yeah, I glossed over that too now. The naming does suggest some sort of creation. I'll put it on my list to try to make the intention clearer.
By the way, as
default_config_dirreturnsOption<PathBuf>, I madeopts.config_dircallclone()to match these types. Maybe, I can delete thisclone()by refactoringdefault_config_dir, but it should be done in another PR.
I would love that, as I am also unhappy about the state of default_config_dir, so feel free to open a PR on that, if you want to!
|
Please ping me if rebase is required! |
| } | ||
| } | ||
|
|
||
| let config_dir = opts.config_dir.clone().or_else(install::default_config_dir); |
There was a problem hiding this comment.
I misunderstood the behavior of
default_config_dir, which I thought would create a directory.
default_config_dirjust indicates a possible configuration directory, so it should be called here as the previous implementation.
Yeah, I glossed over that too now. The naming does suggest some sort of creation. I'll put it on my list to try to make the intention clearer.
By the way, as
default_config_dirreturnsOption<PathBuf>, I madeopts.config_dircallclone()to match these types. Maybe, I can delete thisclone()by refactoringdefault_config_dir, but it should be done in another PR.
I would love that, as I am also unhappy about the state of default_config_dir, so feel free to open a PR on that, if you want to!
Previously, a config file was loaded within `start()`, and if the config file is invalid, Zellij was supposed to show a user what's wrong with it. However, since `start()` starts setting up its terminal with an alternative screen buffer, neither standard output nor standard error could display such an error. This change intends to address this issue by making Zellij load a config file before `start()`. In addition, the patch also includes some refactorings: * Redefine `from_cli_config` with `TryFrom`, which was introduced in Rust 1.34 * Remove conditional declaration `cfg(not(test))` because `start()` now receive a `Config` as the third argument * Introduce [`tempfile`](https://crates.io/crates/tempfile) in order to run tests with actual files * Typo?: "Deserialisation" -> "Deserialization"
|
Thank you. I rebased and squashed commits into a single commit! |
Close #414
Previously, a config file was loaded within
start(), and if the config file is invalid, Zellij was supposed to show a user what's wrong with it. However, sincestart()starts setting up its terminal with an alternative screen buffer, neither standard output nor standard error could display such an error.This change intends to address this issue by making Zellij load a config file before
start().In addition, the patch also includes some refactorings:
from_cli_configwithTryFrom, which was introduced in Rust 1.34cfg(not(test))becausestart()now receive aConfigas the third argumenttempfilein order to run tests with actual files