-
Notifications
You must be signed in to change notification settings - Fork 619
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
Update cli.rs #11148
Update cli.rs #11148
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.
Hey,
Thanks for the contribution. Overall the goal looks commendable and the direction seems right.
Unfortunately I don't think we can merge this as-is. This change needs a fair bit of clean-up. I pointed out some things in-line.
#[clap(long)] | ||
target_home: PathBuf, | ||
/// file containing an optional secret as generated by the |
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 were these descriptions removed?
#[clap(long)] | ||
config_path: Option<PathBuf>, | ||
} | ||
|
||
impl RunCmd { | ||
fn run(self) -> anyhow::Result<()> { | ||
//returning a Result instead of unwrapping, to handle any error properly |
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.
Please follow the rustdoc documentation guidelines. Some things that stand out immediately:
=====
is very seldom used -- this is my first time seeing it used for Rust documentation;///
denotes a doc-comment and should be used here;- I don't think code comments need to motivate a specific return value as you did here, nor they should be used to keep a changelog to the functions – we have git for that after all.
@@ -67,16 +61,17 @@ impl RunCmd { | |||
} | |||
None | |||
}; | |||
|
|||
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.
Trailing whitespace is unnecessary. Have you run cargo fmt
after making your changes?
) | ||
.global(); | ||
|
||
/* |
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 generally strive to not have commented-out code… You probably should remove this.
// This makes it safe to move out of the captured variable `runtime`, which is done by a trick | ||
// using a `swap` of `Cell<Option<Runtime>>`s. | ||
let runtime_cell = Cell::new(Some(runtime)); | ||
//using tokio::runtime::Runtime directly |
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.
Same comment here as above.
runtime_cell.swap(&r); | ||
r.into_inner().unwrap() | ||
}) | ||
} | ||
|
||
//MirrorCommand::run is now asynchronous, and it awaits the result of the RunCmd::run method when applicable. |
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.
Same comment as above.
Summary
RunCmd::run
now returns aResult
instead of unwrapping, ensuring that any errors are properly handled.RunCmd::run
method is now marked as asynchronous and properly awaits the asynchronous operations within it.MirrorCommand::run
is now asynchronous, and it awaits the result of theRunCmd::run
method when applicable.tokio::runtime::Runtime
directly. for consistent error handling.