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.

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

Harmonize BeforeError handling #1117

Merged
merged 2 commits into from Apr 28, 2020
Merged

Conversation

@creekorful
Copy link
Contributor

@creekorful creekorful commented Apr 27, 2020

What type of PR is this?

  • bug
  • cleanup
  • documentation
  • feature

What this PR does / why we need it:

The error handling using BeforeFunc is now harmonized between App#RunContext, App#RunAsSubcommand & Command#Run.

Which issue(s) this PR fixes:

Fixes #1081

Release Notes

(REQUIRED)

Error handling using BeforeFunc is now harmonized between App#RunContext, App#RunAsSubcommand and Command#Run.
@creekorful creekorful requested a review from urfave/cli as a code owner Apr 27, 2020
@creekorful creekorful requested review from lynncyrin and asahasrabuddhe and removed request for urfave/cli Apr 27, 2020
@rliebz
Copy link
Member

@rliebz rliebz commented Apr 28, 2020

Thanks for following up! This is great.

I did a quick audit of the code base, it looks like there's one more BeforeFunc that prints command help in case of errors:

cli/command.go

Lines 150 to 157 in 4bb97e0

if c.Before != nil {
err = c.Before(context)
if err != nil {
_ = ShowCommandHelp(context, c.Name)
context.App.handleExitCoder(context, err)
return err
}
}

Would you mind dropping the call to ShowCommandHelp there as well for consistency?

@creekorful
Copy link
Contributor Author

@creekorful creekorful commented Apr 28, 2020

Thanks for following up! This is great.

I did a quick audit of the code base, it looks like there's one more BeforeFunc that prints command help in case of errors:

cli/command.go

Lines 150 to 157 in 4bb97e0

if c.Before != nil {
err = c.Before(context)
if err != nil {
_ = ShowCommandHelp(context, c.Name)
context.App.handleExitCoder(context, err)
return err
}
}

Would you mind dropping the call to ShowCommandHelp there as well for consistency?

Yup, my bad should have seen this one.

Done !

@creekorful
Copy link
Contributor Author

@creekorful creekorful commented Apr 28, 2020

Btw is it wanted that this logic is uncoupled and not centralised?

IMHO since the BeforeError handling is common (and this is wanted) between the 3 occurrences, I would have centralise the logic somewhere

@rliebz
rliebz approved these changes Apr 28, 2020
@rliebz rliebz merged commit 464c868 into urfave:master Apr 28, 2020
11 of 12 checks passed
11 of 12 checks passed
ubuntu-latest @ Go 1.12
Details
ubuntu-latest @ Go 1.13
Details
ubuntu-latest @ Go 1.14
Details
macos-latest @ Go 1.12
Details
macos-latest @ Go 1.13
Details
macos-latest @ Go 1.14
Details
windows-latest @ Go 1.12
Details
windows-latest @ Go 1.13
Details
windows-latest @ Go 1.14
Details
test-docs
Details
codecov/project 73.24% (+-0.04%) compared to 7a39010
Details
codecov/patch Coverage not affected when comparing 7a39010...c261e4d
Details
@rliebz
Copy link
Member

@rliebz rliebz commented Apr 28, 2020

There's a lot of logic like this that I'd like to see centralized. There are subtle reasons why it's not that way right now, such as repeated logic for different types or handling named return parameters. Not that it can't be improved—it absolutely can be. But it's not necessarily easy.

@rliebz rliebz mentioned this pull request May 5, 2020
4 of 7 tasks complete
@VirrageS VirrageS mentioned this pull request May 5, 2020
1 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants