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

internal/flags: condense case/switch statement #28701

Closed

Conversation

axelKingsley
Copy link

First noticed in #28692

The switch statement in flags.go uses a duplicate imperative for each case. This PR removes the duplicate code per case and merges the various flag types into a single case.

@fjl
Copy link
Contributor

fjl commented Dec 18, 2023

This is not possible in Go. Would be nice if it was, but it's not.

@fjl fjl closed this Dec 18, 2023
@axelKingsley
Copy link
Author

Augh, you're right, thank you for looking at this. Wanted to leave a bit of detail here in case anyone sees this thread and wants to understand why this isn't possible.

In golang, it is possible to group multiple cases together and give them a single imperative: https://go.dev/play/p/cmN3qVOw31b

However, this is not possible in this specific case because this switch uses flag.(type), a special assertion for switch statements. Casting flag := flag.(type) is important because the shared interface cli.flag does not include EnvVars. However, each concrete type (StringFlag, BoolFlag, etc) does have an EnvVars.

My implementation, grouping up the types as a single case will not work because in that situation, the underlying object we're switching on hasn't been cast into any concrete types, and is just a cli.flag without an EnvVars.

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