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.

Sign up for GitHub

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

Jump to bottom

Add StringMap flag #1590

Merged
merged 2 commits into from
Dec 9, 2022
Merged

Add StringMap flag #1590

merged 2 commits into from
Dec 9, 2022

Conversation

avorima
Copy link
Contributor

@avorima avorima commented Nov 18, 2022 •
edited
Loading

What type of PR is this?

  • feature

What this PR does / why we need it:

Add StringMap and StringMapFlag types that are backed by a map[string]string and parse comma-separated key=value pairs.

This is an analogue of pflag's StringToString.

Which issue(s) this PR fixes:

Fixes #1580

Special notes for your reviewer:

I mostly repurposed code from StringSlice. The tests were especially confusing to me since it seems a lot of it is duplicated.

Testing

Created a sample app that uses the flag.

package main

import (
	"fmt"
	"os"

	cli "github.com/urfave/cli/v3"
)

func main() {
	app := cli.NewApp()
	app.Flags = []cli.Flag{
		&cli.StringMapFlag{Name: "kv", EnvVars: []string{"KV"}},
	}
	app.Action = func(c *cli.Context) error {
		dict := c.StringMap("kv")
		fmt.Println(dict)
		return nil
	}

	app.Run(os.Args)
}

Release Notes

Add StringMapFlag that parses comma-separated key=value pairs

@dearchap
Copy link
Contributor

@avorima the changes look good . Can you rebase off of 3.x since we are not adding new flags in 2.x anymore

@avorima
Copy link
Contributor Author

avorima commented Nov 28, 2022

@dearchap What branch is that, v3-dev-main?

@dearchap
Copy link
Contributor

main

@avorima avorima force-pushed the add-string-map-flag branch 5 times, most recently from 4b5281d to 7466f42 Compare November 28, 2022 18:53
@avorima
Copy link
Contributor Author

avorima commented Nov 28, 2022

Code generation is somewhat broken by the changes to documentation in 1.19

@avorima avorima force-pushed the add-string-map-flag branch 3 times, most recently from fe265f6 to fe9471c Compare December 2, 2022 08:35
@avorima
Copy link
Contributor Author

avorima commented Dec 2, 2022

Yeah, so there definitely is a difference when generating code with 1.18 and 1.19. I went with 1.19 now.

@avorima avorima marked this pull request as ready for review December 2, 2022 08:56
@avorima avorima requested a review from a team as a code owner December 2, 2022 08:56
dearchap
dearchap reviewed Dec 3, 2022
View reviewed changes
flag_test.go Show resolved Hide resolved
dearchap
dearchap reviewed Dec 3, 2022
View reviewed changes
flag_map_impl.go
if reflect.TypeOf(t).Kind() == reflect.String {
return fmt.Sprintf("%v", v)
}
return fmt.Sprintf("%T{%s}", v, i.ToString(v))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a simple test case to test map[string]int to enable codecov of this line ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking about this, but shouldn't that be added together with IntMap or similar?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah you could do that.

dearchap
dearchap reviewed Dec 3, 2022
View reviewed changes
flag_map_impl.go Outdated
if !ok {
return fmt.Errorf("item %q is missing separator %q", item, defaultMapFlagKeyValueSeparator)
}
if err := i.value.Set(strings.TrimSpace(value)); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

would we ever need to trimspace for this kind of flags ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's easy to do something like ./prog --myflag "foo=$BAR" and BAR containing unwanted whitespace because it's a script output or whatever. On the other hand removing whitespace can also be delegated to the users of the library. How about I add a TrimSpace flag field that controls this behavior?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure that would be good. Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I added it via the flag config. I had to add it to all string related flags to make the compiler happy

dearchap
dearchap reviewed Dec 3, 2022
View reviewed changes
flag_impl.go Outdated
@@ -192,7 +192,8 @@ func (f *FlagBase[T, C, V]) RunAction(ctx *Context) error {
// IsSliceFlag returns true if the value type T is of kind slice
func (f *FlagBase[T, C, VC]) IsSliceFlag() bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to rename this to IsMultiValueFlag as we are overloading this function name now . I can make that change in a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

@avorima PR #1615

@avorima avorima force-pushed the add-string-map-flag branch 2 times, most recently from 5dd5dd0 to 2dac090 Compare December 5, 2022 22:05
@avorima
Add StringMap flag
9accc2a 
StringMap wraps a `map[string]string` and parses inputs in the form of
"key=value".
@avorima
Add StringConfig for all string flags
e8f968a 
This config allows configuring to automatically trim whitespace of
string value flags. This includes slices and maps.

I had to plumb this down to the string flag level, because of compiler
errors.
@dearchap dearchap merged commit 68a382f into urfave:main Dec 9, 2022
@avorima avorima deleted the add-string-map-flag branch December 13, 2022 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dearchap dearchap dearchap approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add StringMap flag
2 participants
@avorima @dearchap