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

What's the best practice to report an error when using a bool flag in the wrong way? #2057

Open
blockchaindevsh opened this issue Feb 12, 2025 · 10 comments
Assignees
Labels
area/v2 relates to / is being considered for v2 kind/question someone asking a question status/triage maintainers still need to look into this

Comments

@blockchaindevsh
Copy link

blockchaindevsh commented Feb 12, 2025

The --attack is a bool flag, but when run as:

./bin/op-challenger move -attack 1 --claim 0xffff --private-key $PK

--attack 1 makes the remaining parameters empty(e.g., --private-key will be interpreted as empty), which will cause confusion to users.

What's the best way to notify user that the 1 behind --attack is unnecessary and will cause trouble in this situation?

@blockchaindevsh blockchaindevsh added area/v2 relates to / is being considered for v2 kind/question someone asking a question status/triage maintainers still need to look into this labels Feb 12, 2025
@Skeeve
Copy link
Contributor

Skeeve commented Feb 12, 2025

Maybe you should provide more info? I, for my part, have no clue what op-challenger is and what its source code, defining parameters and commands is.

@blockchaindevsh
Copy link
Author

blockchaindevsh commented Feb 12, 2025

The op-challenger binary is irrelevant here; It has a attack flag which is defined like this:

	AttackFlag = &cli.BoolFlag{
		Name:    "attack",
		Usage:   "An attack move. If true, the defend flag must not be set.",
	}

@Skeeve
Copy link
Contributor

Skeeve commented Feb 12, 2025

That's not sufficient. You should provide the full cli definition.

OTOH: --attack is a bool flag. Correct me if I'm wrong, but bool flags do not expect any parameters. So the 1 is not a parameter to --attack but simply a command argument.

So when you check your arguments, you will have 1 as the first and --private-key as the next.

So maybe check you arguments for anything which looks like a flag?

@blockchaindevsh
Copy link
Author

blockchaindevsh commented Feb 12, 2025

That's not sufficient. You should provide the full cli definition.

The full definition is here but as I said it's irrelevant.

OTOH: --attack is a bool flag. Correct me if I'm wrong, but bool flags do not expect any parameters. So the 1 is not a parameter to --attack but simply a command argument.

So when you check your arguments, you will have 1 as the first and --private-key as the next.

So maybe check you arguments for anything which looks like a flag?

Yeah that's correct, but my question is about how to automatically remind the user that he/she has specified --attack 1 by mistake? Or some kind of warning at least.

@Skeeve
Copy link
Contributor

Skeeve commented Feb 12, 2025

The user did not specify --attack 1. The user specified --attack and gave a command line parameter, maybe a filename or a count with the string representation of 1.

So there is no automated way to achieve what you ask for. It's your task to inform the user.

Any program I would write would happily accept the 1 as whatever command line argument I expect at that position. Let's assume I'd expect a filename here, it would either open a file called 1 or complain if it couldn't.

The next parameter --private-key would then be interpreted as the next command line argument and maybe my program would then complain about that parameter not looking like the expected argument.

Just a hint: you're using v2 which is, afaik, no longer in development.

@blockchaindevsh
Copy link
Author

I can understand there's no way to reliably detect it 100%.

But if there's a 1 after a bool flag , followed by other flags, it's very probably a mistake, I've met this kind of mistake before several times. So if cli can do some warning, it'll be very helpful.

@Skeeve
Copy link
Contributor

Skeeve commented Feb 12, 2025

So if cli can do some warning, it'll be very helpful.

So go ahead, switch tocli/v3, implement such a warning and make it a Pull Request.

OTOH: I do not think this is very useful, because you will be unable to use e.g. the filename 1or 0 after a bool flag.

@blockchaindevsh
Copy link
Author

blockchaindevsh commented Feb 12, 2025

So go ahead, switch tocli/v3, implement such a warning and make it a Pull Request.

It's a big project not owned by me:) I just met this issue while using it lol.

OTOH: I do not think this is very useful, because you will be unable to use e.g. the filename 1or 0 after a bool flag.

We can always improve the auto-detection during iterations.

@Skeeve
Copy link
Contributor

Skeeve commented Feb 12, 2025

So if it's not your project, what's the reason for asking for improvements in cli/v2?

We can always improve the auto-detection during iterations.

Who is "we"? If "we" includes "you", go ahead and create a PR for this auto-detection in cli/v3. While it won't be of use for the project you're interested in, it will give you what you ask for in the next iteration of cli. If you don't do it, I doubt anyone will pick this up.

@blockchaindevsh
Copy link
Author

blockchaindevsh commented Feb 12, 2025

So if it's not your project, what's the reason for asking for improvements in cli/v2?

Because I see it as a general issue when using cli flags.

I don't have the bandwidth to do a PR now, but may do it in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/v2 relates to / is being considered for v2 kind/question someone asking a question status/triage maintainers still need to look into this
Projects
None yet
Development

No branches or pull requests

2 participants