fix(main): Refactor src/main.rs by splitting one massive main function into commands.rs

[ken-matsui]

What I did:

• Split one massive main function in src/main.rs into multiple functions in src/commands.rs.
• Introduced dialoguer to bring a confirmation prompt ([y/n]) for the following code. (I thought it is better to use a well-maintained library instead of implementing low-code in the same function.)

zellij/src/main.rs

Lines 46 to 60 in 043a3cf

	use std::io::{stdin, stdout, Write};
	let mut answer = String::new();
	println!("WARNING: this action will kill all sessions.");
	print!("Do you want to continue? [y/N] ");
	let _ = stdout().flush();
	stdin().read_line(&mut answer).unwrap();
	match answer.as_str().trim() {
	"y" | "Y" | "yes" | "Yes" => kill_all_sessions(sessions),
	_ => {
	println!("Abort.");
	process::exit(1);
	}
	}

NOTE: This PR depends on:

• 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

[jaeheonji]

I implemented the confirmation prompt function as simply as possible. I agree that if there is a good library, it is better to use it 👍

[ken-matsui]

@jaeheonji
I am sorry to bother you... Anyway, thank you so much for working on this changes :)

[jaeheonji]

@ken-matsui Oh! No need to be sorry! rather, thank you for suggesting a better alternative.

[a-kenji]

This looks good to me,
thanks for the great work!
The main function is a lot clearer now.
Here is the same issue as in #823,
but I'll approve this here since apart from that I don't have any qualms.
I will wait with the merge though until #823 is fixed.

[ken-matsui]

@a-kenji
Thanks for your review!

I found one question regarding the Zellij lifecycle. The main function I refactored is:

if let Some(Command::Sessions(Sessions::ListSessions)) = opts.command {
    commands::list_sessions();
}
if let Some(Command::Sessions(Sessions::KillAllSessions { yes })) = opts.command {
    commands::kill_all_sessions(yes);
}
if let Some(Command::Sessions(Sessions::KillSession { ref target_session })) = opts.command {
    commands::kill_session(target_session);
}
if let Some(path) = opts.server {
    commands::start_server(path);
} else {
    commands::start_client(opts);
}

I do not understand how the Zellij lifecycle works, but is there a possibility that Zellij sequentially executes multiple commands at the same lifecycle for example commands::start_client after commands::list_sessions?

If the answer is no, we can remove all wasteful if-evaluations to improve performance like the following.

if let Some(Command::Sessions(Sessions::ListSessions)) = opts.command {
    commands::list_sessions();
} else if let Some(Command::Sessions(Sessions::KillAllSessions { yes })) = opts.command {
    commands::kill_all_sessions(yes);
} else if let Some(Command::Sessions(Sessions::KillSession { ref target_session })) = opts.command {
    commands::kill_session(target_session);
} else if let Some(path) = opts.server {
    commands::start_server(path);
} else {
    commands::start_client(opts);
}

[a-kenji]

@ken-matsui
That is a good catch,
for now I do think these commands can't be executed sequentially.

[jaeheonji]

We currently have session-related functionality in sessions.rs.

So, how about changing the name of the function to execute_kill_session, and the kill_session_impl inside the function importing the function kill_session of sessions.rs?

In my personal opinion, I think this would make more sense for file names.

[ken-matsui]

@jaeheonji
Thanks for your suggestion!

How do you think the naming convention won't be unified if renaming kill_session to execute_kill_session by seeing it from the main function? If you would do so, all such as start_server should be renamed with execute_start_server respectively.

[jaeheonji]

The first idea was to prevent (more clarify) the name of
kill_session from being used in two places(sessions.rs and commands.rs). But as you told me, it looks right to change the name of start_server for unity. I want to respect your opinion here. What do you think it's better to do?

[ken-matsui]

@jaeheonji
I personally mainly do not care about duplication of function names. I would think that's why there is a module system and visibility. We can rename it with as or import an upper namespace instead.

However, this is just my personal view. I think we need code owners' opinions. How do you think about this problem, @a-kenji ?

[a-kenji]

@ken-matsui
I currently prefer the simpler names here, especially since in the main function the commands are already called with:

commands::kill_session(target_session);

If it turns out to be confusing we can still change it in the future.
Thanks for the insight here @jaeheonji.

[jaeheonji]

(Don't mind about the name of the variable. just example)

if sessions.is_empty() {
...
} else {
  let mut run = true
  if !yes {
    run = Confirm::new()
      .with_prompt("...")
      .interact()
      .unwrap()
  }

  if !run {
    println!("Abort.");
    process:exit(1);
  }

  // run `kill_session` as a loop
}

I don't know if this works normally, it looks like you don't need any additional functions. I want to know your opinion :)

[ken-matsui]

@jaeheonji
Thanks! That would be clearer. I will improve the kill_all_sessions function 🙌

[ken-matsui]

I updated this at dfb3b9d without bringing a new variable and mutable :)

[jaeheonji]

It looks really good 👍

[ken-matsui]

@a-kenji
Thanks for your answer. I will rewrite the main function with optimized if-evaluations.

[a-kenji]

@ken-matsui,
Thank you for this awesome work!
This looks so much better now.
It is ready, or? I will merge it in the next days.

[ken-matsui]

@a-kenji
Thanks! This PR has been squashed and is ready, but I and @jaeheonji would like you to see this #829 (comment).

[a-kenji]

Thank you @ken-matsui for taking the time in writing this awesome pr!
It does look much cleaner now.
And also thanks @jaeheonji for the great input!