fix(main): Refactor src/main.rs by splitting one massive main function into commands.rs by ken-matsui · Pull Request #829 · zellij-org/zellij

ken-matsui · 2021-11-01T18:12:45Z

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

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 · 2021-11-02T02:18:39Z

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 · 2021-11-02T05:26:35Z

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

jaeheonji · 2021-11-02T05:50:31Z

@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 · 2021-11-02T19:12:03Z

@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 · 2021-11-03T12:02:27Z

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

jaeheonji · 2021-11-02T06:20:26Z

+    }
+}
+
+pub(crate) fn kill_session(target_session: &Option<String>) {


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.

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

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?

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

@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 · 2021-11-02T06:36:43Z

+                process::exit(1);
+            } else if yes {
+                kill_all_sessions_impl(sessions);
+            } else {


(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 :)

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

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

It looks really good 👍

ken-matsui · 2021-11-04T04:48:31Z

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

a-kenji · 2021-11-05T19:33:07Z

@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 · 2021-11-05T21:38:29Z

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

…n into commands.rs * fix(main): Remove unnecessary pub visibility from the main function in `src/main.rs` * fix(main): Avoid unnecessary if-evaluations in the main function of `src/main.rs` * fix(commands): Simplify kill_all_sessions

a-kenji · 2021-11-05T22:05:17Z

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!

a-kenji mentioned this pull request Nov 1, 2021

feat: add initial session name to layout template #789

Merged

a-kenji mentioned this pull request Nov 2, 2021

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

Merged

a-kenji approved these changes Nov 2, 2021

View reviewed changes

jaeheonji reviewed Nov 3, 2021

View reviewed changes

jaeheonji approved these changes Nov 4, 2021

View reviewed changes

a-kenji approved these changes Nov 5, 2021

View reviewed changes

a-kenji merged commit 510feb3 into zellij-org:main Nov 5, 2021

ken-matsui deleted the refactor-src-main branch November 5, 2021 22:37

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

Uh oh!

Conversation

ken-matsui commented Nov 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaeheonji commented Nov 2, 2021

Uh oh!

ken-matsui commented Nov 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaeheonji commented Nov 2, 2021

Uh oh!

a-kenji left a comment

Choose a reason for hiding this comment

Uh oh!

ken-matsui commented Nov 2, 2021

Uh oh!

a-kenji commented Nov 3, 2021

Uh oh!

jaeheonji Nov 2, 2021

Choose a reason for hiding this comment

Uh oh!

ken-matsui Nov 4, 2021

Choose a reason for hiding this comment

Uh oh!

jaeheonji Nov 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ken-matsui Nov 4, 2021

Choose a reason for hiding this comment

Uh oh!

a-kenji Nov 5, 2021

Choose a reason for hiding this comment

Uh oh!

jaeheonji Nov 2, 2021

Choose a reason for hiding this comment

Uh oh!

ken-matsui Nov 4, 2021

Choose a reason for hiding this comment

Uh oh!

ken-matsui Nov 4, 2021

Choose a reason for hiding this comment

Uh oh!

jaeheonji Nov 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ken-matsui commented Nov 4, 2021

Uh oh!

a-kenji commented Nov 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ken-matsui commented Nov 5, 2021

Uh oh!

a-kenji commented Nov 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ken-matsui commented Nov 1, 2021 •

edited

Loading

ken-matsui commented Nov 2, 2021 •

edited

Loading

jaeheonji Nov 4, 2021 •

edited

Loading

jaeheonji Nov 4, 2021 •

edited

Loading

a-kenji commented Nov 5, 2021 •

edited

Loading