feat(attach): Support --index option for attach sub-command to choose the session indexed by provided number in the active sessions ordered creation date

[ken-matsui]

This PR is related to the following issue.

• Feature: Add --index option for attach sub-command to choose the session indexed by provided number in the active sessions ordered creation date #823

I didn't think it is better to have a short form of --first option because -f is often used for --force option even though attach sub-command does not have it.

The main function is messy, and some codes have duplicates; I think splitting the main function into multiple functions would be better. If you want to do so, I would like to refactor it in another PR.

[a-kenji]

Thanks for creating this pr! I hope I will get some time to look at it soon.

I didn't think it is better to have a short form of --first option because -f is often used for --force option even though attach sub-command does not have it.

I agree.

The main function is messy, and some codes have duplicates; I think splitting the main function into multiple functions would be better. If you want to do so, I would like to refactor it in another PR.

I personally would welcome that very much.

[ken-matsui]

I personally would welcome that very much.

Thank you for accepting my suggestion.
I will work on this refactoring.

[ken-matsui]

@a-kenji
Would you mind if I replace get_sessions_sorted_by_creation_date and print_sessions_with_index with get_sessions and print_sessions respectively? If you do not mind this, I would like to create a new PR derived from this because the object of this PR will be far from clear. If you want to keep splitting them, I would like to merge duplicates when I do refactoring.

[a-kenji]

@ken-matsui
I think that should be fine.
As long as we can reliably get either the index, or the name,
or both from the functions I am all for the deduplication.

I think #789 should be soon ready to be merged,
and I hope to get it in prior to any changes here -
just to give a heads up for possible merge conflicts.

[ken-matsui]

@a-kenji
Thank you for your response and advance notice of conflicts.

[a-kenji]

@ken-matsui
Please ping me if this is ready for a review.

[ken-matsui]

@a-kenji
This and refactor PRs are ready for review ;)

[a-kenji]

Hey!
This looks good.
I think there is a small issue with error propagation,
but in general I am fine with everything here.

[a-kenji]

I think it would be best for the file operations that could reasonably fail to use the try ? operator here.
In the command.rs of #829 you already use the potentially propagated error.
That could explain possible failures.

For example I can't use this feature in its current state, because my file system doesn't support creation time.
It would be nice to not panic in that instance:

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Error { kind: Uncategorized, message: "creation time is not available for the filesystem" }', src/sessions.rs:40:74

[ken-matsui]

@a-kenji
Thanks for your review :)

To introduce ? operator here, it would be a problem that an error type of file.matadata() and io::ErrorKind are much different. However, they can be into one error type using anyhow library. Would it be acceptable to add it?

By bringing it, get_sessions_sorted_by_creation_date will be:

-pub(crate) fn get_sessions_sorted_by_creation_date() -> Result<Vec<String>, io::ErrorKind> {
+pub(crate) fn get_sessions_sorted_by_creation_date() -> anyhow::Result<Vec<String>> {
    match fs::read_dir(&*ZELLIJ_SOCK_DIR) {
        Ok(files) => {
            let mut sessions_with_creation_date: Vec<(String, SystemTime)> = Vec::new();
-           files.for_each(|file| {
+           for file in files {
-               let file = file.unwrap();
+               let file = file?;
                let file_name = file.file_name().into_string().unwrap();
-               let file_created_at = file.metadata().unwrap().created().unwrap();
+               let file_created_at = file.metadata()?.created()?;
-               if file.file_type().unwrap().is_socket() && assert_socket(&file_name) {
+               if file.file_type()?.is_socket() && assert_socket(&file_name) {
                    sessions_with_creation_date.push((file_name, file_created_at));
                }
-           });
+           }
            sessions_with_creation_date.sort_by_key(|x| x.1); // the oldest one will be the first

            let sessions = sessions_with_creation_date
                .iter()
                .map(|x| x.0.clone())
                .collect();
            Ok(sessions)
        }
        Err(err) => {
            if let io::ErrorKind::NotFound = err.kind() {
                Ok(Vec::with_capacity(0))
            } else {
-               Err(err.kind())
+               Err(err.into())
            }
        }
    }
}

Many unwrap disappeared except for OsString::into_string which actually does not have an error type.

To properly treat and propagate errors, I think almost all unwrap should be removed and replaced with anyhow. So, I suggest doing refactor all functions, and some situation needs to introduce thiserror as well. Additionally, the main function also can return anyhow::Result as shown here.

[a-kenji]

Oh, I did not realize the metadata error was so much different!
Yes, I think adding anyhow should be fine.

For which situations do you think thiserror needs to be used?

[ken-matsui]

@a-kenji

Oh, I did not realize the metadata error was so much different!
Yes, I think adding anyhow should be fine.

Thanks!

For which situations do you think thiserror needs to be used?

anyhow requires an error type to implement std::error::Error trait. This is why I could not apply ? operator to OsString::into_string. We, eventually, come to want to create a custom error type, but many traits such as fmt::Display, std::error::Error, and From<_> need implementing.

Accordingly, by introducing thiserror, most implementation can be one enum declaration like the example below; for example, the ConfigError can be much improved:

https://docs.rs/thiserror/1.0.30/thiserror/index.html#example

Besides, it would be good to organize scattered errors on Zellij.

[ken-matsui]

If you want, I can add thiserror for another refactor to improve ConfigError 👍

[a-kenji]

Thank you for that explanation,
if it makes custom errors easier, I am all for it.

I would gladly look at a pr that improves and makes the
error structure easier.

[ken-matsui]

Thank you so much. I will commit to it :)

[a-kenji]

@ken-matsui
Thank you, I think this looks great.
I will merge it if you say it is ready.
If you want to squash it you can do it.

[ken-matsui]

@a-kenji
Thank you so much for your review! This PR is ready.

Do you recommend squashing it?
I usually don't do it unless repository owners instruct me to do it.

[a-kenji]

@ken-matsui
Awesome, good to hear!
Yes currently we squash merge commits,
we think the history might be easier to navigate for the time being.

[a-kenji]

Thank you!

[ken-matsui]

Oh, thank you so much for merging this pr!