Skip to content
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.

Sign up for GitHub

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

fix(complete): Use "expect" instead of "unwrap" #3322

Closed
wants to merge 1 commit into from
Closed

fix(complete): Use "expect" instead of "unwrap" #3322

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion clap_complete/src/shells/bash.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@ impl Generator for Bash {
}

fn generate(&self, app: &App, buf: &mut dyn Write) {
let bin_name = app.get_bin_name().unwrap();
let bin_name = app
.get_bin_name()
.expect("Getting the App's \"bin_name\" failed");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reads like its describing app.get_bin_name().unwrap() and isn't adding any value.

A tip for writing expects is to use them to document what the expected pre-condition is that failed. In this case, crate::generate set the bin_name

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and isn't adding any value.

i mean its still better than unwrap failed at, this way you currently have at least the clue what property is missing before having to investigate the code that threw the message

but yes, the message is not really great

how about changing the message to:
Getting the App's \"bin_name\" failed: did crate::generate set the \"bin_name\" correctly?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The distinction is expects are not error reporting but a form of assertion. Developers get a traceback telling them where it failed and can easily see what the line is that failed. The emphasis should be on the documentating the invariant (by emphasis, I mean linguistically)

This goes back to the tip I mentioned. A more concrete way of putting it is to turn "expect("...")into "expect ..." and fill in the sentence fragment. In this case "expect crate::generate set the bin_name" which becomesexpect("crate::generate set the bin_name")`. Yes, the panic message isn't optimized for this (its a common complaint of expect's panic message).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expect crate::generate set the bin_name
crate::generate set the bin_name

they dont sound descriptive to me, how about Expected crate::generate to have set "bin_name" and maybe with the addition of (Did this function get run directly?) as a hint to not run this function directly (but through crate::generate)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expected crate::generate to have set "bin_name"

As I said, this is redundant with the line that is failing the assert

(Did this function get run directly?)

This is more of error reporting verbage than assertion violation verbage.


w!(
buf,
Expand Down
4 changes: 3 additions & 1 deletion clap_complete/src/shells/elvish.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@ impl Generator for Elvish {
}

fn generate(&self, app: &App, buf: &mut dyn Write) {
let bin_name = app.get_bin_name().unwrap();
let bin_name = app
.get_bin_name()
.expect("Getting the App's \"bin_name\" failed");

let mut names = vec![];
let subcommands_cases = generate_inner(app, "", &mut names);
Expand Down
4 changes: 3 additions & 1 deletion clap_complete/src/shells/fish.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@ impl Generator for Fish {
}

fn generate(&self, app: &App, buf: &mut dyn Write) {
let command = app.get_bin_name().unwrap();
let command = app
.get_bin_name()
.expect("Getting the App's \"bin_name\" failed");
let mut buffer = String::new();

gen_fish_inner(command, &[], app, &mut buffer);
Expand Down
4 changes: 3 additions & 1 deletion clap_complete/src/shells/powershell.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@ impl Generator for PowerShell {
}

fn generate(&self, app: &App, buf: &mut dyn Write) {
let bin_name = app.get_bin_name().unwrap();
let bin_name = app
.get_bin_name()
.expect("Getting the App's \"bin_name\" failed");

let mut names = vec![];
let subcommands_cases = generate_inner(app, "", &mut names);
Expand Down
4 changes: 3 additions & 1 deletion clap_complete/src/shells/zsh.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,9 @@ _{name}() {{

_{name} \"$@\"
",
name = app.get_bin_name().unwrap(),
name = app
.get_bin_name()
.expect("Getting the App's \"bin_name\" failed"),
initial_args = get_args_of(app, None),
subcommands = get_subcommands_of(app),
subcommand_details = subcommand_details(app)
Expand Down