-
Notifications
You must be signed in to change notification settings - Fork 6
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
om health
: Shell dotfiles are Nix-managed
#306
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rsrohitsingh682 Could you post screenshots of the behaviour on macOS, Linux and on some system where neither bash nor zsh is the current shell?
crates/nix_health/CHANGELOG.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you revert the irrelevant changes to this file?
// Check if shell configurations (zsh or bash) are managed by Nix | ||
are_shell_dotfiles_nix_managed("zsh", &shell_dotfiles) | ||
|| are_shell_dotfiles_nix_managed("bashrc", &shell_dotfiles) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic is wrong here. We want to check the current shell, and current shell only.
Also, what happens if the user is using a different shell like fish
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, 'tis time to refactor this (encapsulate) by introducing a struct
along with struct methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic is wrong here. We want to check the current shell, and current shell only.
@srid I tried using SHELL
environment variable for this but it turns out it gives the default shell of the user not the current active shell. There are other ways to determine the current active shell but those commands doesn't work well with rust. Hence I went ahead with this method for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rsrohitsingh682 Do you understand why the logic is wrong? If I have both bash and zsh dotfiles, and bash dotfiles are managed by Nix but zsh dotfiles are not managed by Nix - then what would be the result of the check if zsh is my current shell?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh! Right!! Got it. I made the check to use OR
condition thinking what if user is just using only one shell either of (zsh or bash). But yeah I should have considered the case that you mentioned.
|
||
// check if dotfile(s) for a given shell points to /nix/store | ||
fn are_shell_dotfiles_nix_managed(shell: &str, dotfiles: &HashMap<&str, Vec<&str>>) -> bool { | ||
let home = env::var("HOME").unwrap_or_default(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens in the default case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wouldn't panic instead will return empty string. I didn't want to throw error here because it wouldn't let the health process to be completed (break in between). Hence I thought if it returns empty string we would eventually show that shell configuration is not managed by nix ( However this is still not correct way).Would need your advice here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not reflexively use Default instance without understanding what it does..
Do you understand how a) an empty string for $HOME is meaningless, and b) how $HOME being empty affects the logic in your function? When (b) happens, is your function still correct?
} else { | ||
false | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function interface should be small enough to not have dummy code paths like this. I believe this will be addressed by introducing struct.
nix_health
: Check Shell configurations are Nix-Managedom health
: Shell dotfiles are Nix-managed
f098426
to
5cc216c
Compare
MacOS where zsh is managed by nix. MacOS where zsh is not managed by nix. Linux (Ubuntu) where bash is not managed by nix. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the struct encapsulation can still be improved (for one thing, you only need one struct not two). But the best way to understand why is to answer this question: How does this behave under the following situations?
- User's shell is neither bash nor zsh. Its dotfiles are not managed by Nix.
- User's shell is neither bash nor zsh. Its dotfiles are managed by Nix.
let shell_name = Path::new(path) | ||
.file_name() | ||
.and_then(|name| name.to_str()) | ||
.unwrap_or(""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you think empty string is better than None
(in the context of the match
below)? What do you think is the purpose of the Option
type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a miss. I didn't realise I could write it like this as well.
fn from_path(path: &str) -> Self {
Path::new(path)
.file_name()
.and_then(|name| name.to_str())
.map(|shell_name| match shell_name {
"zsh" => Shell::Zsh,
"bash" => Shell::Bash,
_ => Shell::Unknown,
})
.unwrap_or(Shell::Unknown)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It still is not faithful to the environment. Are you really sure Shell::Unknown
can never refer to when the user has bash or zsh as their current shell? In your code snippet above, there is in fact a condition under which your function will return Shell::Unknown
despite the user having bash or zsh as their current shell.
Try to understand what each function does, including the nature of the range of values it can return.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall: we need a case to handle unexpected situations and errors, and then report that accordingly instead of giving a false negative.
For both the cases it would return |
@srid The CI seems to be failing for For checking the shell configuration it is looking into
Any thoughts on this ? |
That's because in CI How are you handling the case of a dotfile not even existing in the first place? |
resolves #299