om health: Shell dotfiles are Nix-managed

[rsrohitsingh682]

resolves #299

[srid]

@rsrohitsingh682 Could you post screenshots of the behaviour on macOS, Linux and on some system where neither bash nor zsh is the current shell?

[srid]

Can you revert the irrelevant changes to this file?

[srid]

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?

[srid]

Also, 'tis time to refactor this (encapsulate) by introducing a struct along with struct methods.

[rsrohitsingh682]

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.

[srid]

@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?

[rsrohitsingh682]

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.

[srid]

What happens in the default case?

[rsrohitsingh682]

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.

[srid]

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?

[srid]

Function interface should be small enough to not have dummy code paths like this. I believe this will be addressed by introducing struct.

[rsrohitsingh682]

@rsrohitsingh682 Could you post screenshots of the behaviour on macOS, Linux and on some system where neither bash nor zsh is the current shell?

MacOS where zsh is managed by nix.

MacOS where zsh is not managed by nix.

Linux (Ubuntu) where bash is not managed by nix.

[srid]

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.

[srid]

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?

[rsrohitsingh682]

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)
    } 
 

[srid]

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.

[srid]

Overall: we need a case to handle unexpected situations and errors, and then report that accordingly instead of giving a false negative.

[rsrohitsingh682]

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.

For both the cases it would return false and say it is not managed by Nix which would be wrong for the second part. Now when I think about it I am checking only dotfiles but for shells like fish the configurations are present in ~/.config/fish/config.fish. So ideally I should maintain a list where I can refer to the correct place be it .dotfiles or config files.

[rsrohitsingh682]

@srid The CI seems to be failing for aarch64-darwin while running tests.

For checking the shell configuration it is looking into

/var/lib/github-runners/sambar-juspay-08/.bashrc

Any thoughts on this ?

[srid]

That's because in CI $HOME is /var/lib/github-runners/sambar-juspay-08/.

How are you handling the case of a dotfile not even existing in the first place?