Skip to content

Fixing the incorrect error code even when CLI exits correctly #1151

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

Merged

Conversation

abhishekkumams
Copy link
Contributor

@abhishekkumams abhishekkumams commented Jan 31, 2023

Why make this change?

What is this change?

  • Options --help generates ErrorType.HelpRequestedError and ErrorType.HelpVerbRequestedError while --version generates ErrorType.VersionRequestedError.
  • While parsing we specifically looked if these error types are produced and marked them with exit code 0.

How was this tested?

  • manually
  • unit tests

Sample Request(s)

image

Copy link
Contributor

@Aniruddh25 Aniruddh25 left a comment

Choose a reason for hiding this comment

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

some questions

@Aniruddh25
Copy link
Contributor

Any error to perform init etc should result in -1 exit code.

@Aniruddh25
Copy link
Contributor

dab --version

last exit code should be 0.

Copy link
Contributor

@seantleonard seantleonard left a comment

Choose a reason for hiding this comment

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

nit

@abhishekkumams
Copy link
Contributor Author

abhishekkumams commented Feb 1, 2023

well, I'll create a separate issue to expand the scope, so that it gives -1 for any kind of errors (like validation errors, parsing errors, etc). Keeping in view the release, we should get this in, as swa cli team uses only dab --version to check if dab is installed or not, or if it's on the latest version. Issue: #1162

@Aniruddh25
Copy link
Contributor

well, I'll create a separate issue to expand the scope, so that it gives -1 for any kind of errors (like validation errors, parsing errors, etc). Keeping in view the release, we should get this in, as swa cli team uses only dab --version to check if dab is installed or not, or if it's on the latest version. Issue: #1162

Even if we don't expand the scope now, by removing isParsed we are regressing the existing functionality, where we return -1 if the parsed object is not a recognized command for e.g. edit.
I think we should explicitly checking for version and help as one of the supported commands, return 0 for those and retain existing functionality - no regressions.

@abhishekkumams
Copy link
Contributor Author

abhishekkumams commented Feb 2, 2023

There has been an issue created for System.CommandLine as to understand why --help and --version are considered as error.
Ref: commandlineparser/commandline#630

I'll use the workaround for --help and --version

@abhishekkumams abhishekkumams self-assigned this Feb 2, 2023
@abhishekkumams abhishekkumams added bug Something isn't working cli labels Feb 2, 2023
@abhishekkumams abhishekkumams added this to the Jan2023 milestone Feb 2, 2023
Copy link
Contributor

@seantleonard seantleonard left a comment

Choose a reason for hiding this comment

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

Thanks for this update and making sure no regressions occur. Also appreciate your documenting commandline package behavior to justify and design your workaround!

@Aniruddh25 Aniruddh25 merged commit a375e66 into main Feb 2, 2023
@Aniruddh25 Aniruddh25 deleted the dev/abhishekkuma/fix-incorrect-error-code-with-dab-cli branch February 2, 2023 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cli
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect Error code with dab cli
4 participants