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

Use Cobra functionality to validate arguments #1126

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

elhimov
Copy link
Contributor

@elhimov elhimov commented Mar 13, 2025

Closes #1125

@elhimov elhimov force-pushed the elhimov/gh-1125-use-cobra-to-validate-arguments branch 3 times, most recently from 3cd1fde to 8dd2925 Compare March 14, 2025 10:22
@elhimov elhimov added the full-ci Enables full ci tests label Mar 14, 2025
@elhimov elhimov force-pushed the elhimov/gh-1125-use-cobra-to-validate-arguments branch from 8dd2925 to 3aa3a3f Compare March 14, 2025 11:08
@elhimov elhimov force-pushed the elhimov/gh-1125-use-cobra-to-validate-arguments branch from 3aa3a3f to 9e2f58d Compare March 14, 2025 12:18
@elhimov elhimov force-pushed the elhimov/gh-1125-use-cobra-to-validate-arguments branch 3 times, most recently from 1060718 to 544ca5b Compare March 17, 2025 12:00
@elhimov elhimov force-pushed the elhimov/gh-1125-use-cobra-to-validate-arguments branch from 544ca5b to ff6abda Compare March 18, 2025 07:13
@elhimov elhimov force-pushed the elhimov/gh-1125-use-cobra-to-validate-arguments branch from ff6abda to a4e8bcb Compare March 18, 2025 10:34
}
case 1:

if len(args) > 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

As for me, code with switch len(args) { was a bit clear to understand and support.

Copy link
Contributor

@oleg-jukovec oleg-jukovec Mar 18, 2025

Choose a reason for hiding this comment

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

Yes, it is. Switch instead of multiple if-else it is a Golang pattern. Even something like:

switch {
case foo < 1:
case bar > 2:
case baz == bar && baz == 3:
default:
}

Copy link
Contributor Author

@elhimov elhimov Mar 18, 2025

Choose a reason for hiding this comment

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

As for me this approach is simpler.
Consider them as 2 independent if-else. Each of them just deals with single corresponding parameter.
We don't need to validate program here anymore because its validation was moved to the dedicated validation function (as well as we don't need to duplicate that validation code with error handling in two cases). All we need here is to deal with 2 parameters and it could be done with 2 independent if-else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just in case an important note: it wasn't just changing switch to if-else. The original switch-implementation did both: validated the existent arguments and obtained the missing ones. This PR follows the approach provided by Cobra Framework and put the validation part into the dedicated function. After that the only thing left to do is obtaining the missing arguments (if any). And it can be done completely independent for every of them with simple if-else, so no need in switch.
I'm clearly understand that switch in Go is a fancy thing that allows much more than switch in C, but here just not the case.

@@ -16,7 +16,7 @@ import (

type DownloadCtx struct {
// SDK version to download.
version string
Version string
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please update comment for the field, too. It should start with the field name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@elhimov elhimov force-pushed the elhimov/gh-1125-use-cobra-to-validate-arguments branch from a4e8bcb to 72539ab Compare March 18, 2025 20:00
@elhimov elhimov requested review from dmyger and oleg-jukovec March 19, 2025 07:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
full-ci Enables full ci tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use Cobra functionality to validate arguments.
4 participants