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

Tools 2673 don't print usage information when config validation fails #19

Merged
merged 3 commits into from
Nov 1, 2023

Conversation

dwelch-spike
Copy link
Contributor

No description provided.

@dwelch-spike dwelch-spike self-assigned this Oct 27, 2023
Copy link

@jdogmcsteezy jdogmcsteezy left a comment

Choose a reason for hiding this comment

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

I don't think we need to print the usage information if we can't unmarshal the input file, if we can't open the output file, or if we can't write to the output file. Personally, if they provided syntactically correct input I don't think usage information should be displayed. It is interesting that we need to explicitly specify cmd.SilenceUsage=true. I don't remember needing to do that for the uda. Most of this logic seemed to be handled automatically.

If you think otherwise we can just merge it

@dwelch-spike
Copy link
Contributor Author

I don't think we need to print the usage information if we can't unmarshal the input file, if we can't open the output file, or if we can't write to the output file. Personally, if they provided syntactically correct input I don't think usage information should be displayed. It is interesting that we need to explicitly specify cmd.SilenceUsage=true. I don't remember needing to do that for the uda. Most of this logic seemed to be handled automatically.

If you think otherwise we can just merge it

I think the SilentUsage call is required because I'm using the RunE instead of the Run command method. I'll look into some cleaner ways of doing this. I might disable usage output in the root command and then enable it in input checks.

@jdogmcsteezy jdogmcsteezy self-requested a review October 27, 2023 23:07
@dwelch-spike
Copy link
Contributor Author

dwelch-spike commented Oct 30, 2023

The latest commit unifies error displaying behavior by.

  • Flag parsing errors and invalid commands always print usage. Nothing else does.
  • SilenceErrors is set to true in the root command because errors are displayed by the logger in the Execute function.
  • SilenceUsage is set to true in the root command and instead called by SetFlagErrorFunc.
  • Mostly removed the multiErrors for simplicity's sake.
  • Removed error logs from the convert command because the returned errors are logged in Execute
  • Introduced a SilentError that is used to disable logging for errors that shouldn't be displayed.

Copy link

@jdogmcsteezy jdogmcsteezy left a comment

Choose a reason for hiding this comment

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

LGTM=

@dwelch-spike dwelch-spike merged commit 85acaba into main Nov 1, 2023
2 checks passed
@dwelch-spike dwelch-spike deleted the tools-2673 branch November 1, 2023 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants