feat: add TextFlag #2055

somebadcode · 2025-02-10T20:01:06Z

Added TextFlag which supports setting values for types that satisfies both encoding.TextMarshaller and encoding.TextUnmarshaller which is very handy when you want to set log levels or string-like types that satisfies the interfaces.

Fixes: #2051

What type of PR is this?

feature

What this PR does / why we need it:

Making it easier to set types that satisfies encoding.TextMarshaller and encoding.TextUnmarshaller such as log/slog.LogLevelVar, using flags as see in the standard library's flag.

Which issue(s) this PR fixes:

Fixes #2051

Release Notes

Added `TextFlag` to support setting values for types that satisfy both  `encoding.TextMarshaller` and `encoding.TextUnmarshaller`.

* Added `TextFlag` which supports setting values for types that satisfies the `encoding.TextMarshaller` and `encoding.TextUnmarshaller` which is very handy when you want to set log levels or string-like types that satifies the interfaces. Fixes: urfave#2051 Signed-off-by: Tobias Dahlberg <[email protected]>

Signed-off-by: Eng Zer Jun <[email protected]>

Juneezee

Thanks for your contribution! 😊

Just a few comments regarding the tests. If possible, please add a few more test cases to cover edge cases and improve coverage.

flag_text_test.go

Juneezee · 2025-02-11T10:06:03Z

flag_text_test.go

+			if err := tt.flag.Apply(set); err != nil {
+				t.Fatalf("Apply(%v) failed: %v", tt.args, err)
+			}
+
+			err := set.Parse(tt.args)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("Parse() error = %v, wantErr %v", err, tt.wantErr)
+
+				return
+			} else if (err != nil) == tt.wantErr {
+				// Expected error.
+				return
+			}
+
+			if got := tt.flag.GetValue(); got != tt.want {
+				t.Errorf("Value = %v, want %v", got, tt.want)
+			}


Use the testify package for assertions

I don't use testify and I don't see it as idiomatic Go.
But I've added it and I hope that I'm using it the correct way.

The tests should now have full coverage.

dearchap · 2025-02-11T14:42:54Z

flag_text.go

+	return v.Value
+}
+
+func (v TextValue) Create(t TextMarshalUnMarshaller, _ *TextMarshalUnMarshaller, c StringConfig) Value {


Suggested change

func (v TextValue) Create(t TextMarshalUnMarshaller, _ *TextMarshalUnMarshaller, c StringConfig) Value {

func (v TextValue) Create(t TextMarshalUnMarshaller, p *TextMarshalUnMarshaller, c StringConfig) Value {

b, _ := p.MarshalText()

t.UnmarshalText(b)

I've made the necessary changes.

dearchap · 2025-02-11T14:43:43Z

flag_text.go

+type TextFlag = FlagBase[TextMarshalUnMarshaller, StringConfig, TextValue]
+
+type TextValue struct {
+	Value  TextMarshalUnMarshaller


Suggested change

Value TextMarshalUnMarshaller

Value *TextMarshalUnMarshaller

I've made the necessary changes.

dearchap · 2025-02-11T14:43:57Z

flag_text.go

+
+func (v TextValue) Create(t TextMarshalUnMarshaller, _ *TextMarshalUnMarshaller, c StringConfig) Value {
+	return &TextValue{
+		Value:  t,


Suggested change

Value: t,

Value: p,

I've made the necessary changes.

dearchap · 2025-02-11T14:44:44Z

flag_text.go

+
+func (v TextValue) Set(s string) error {
+	if v.Config.TrimSpace {
+		return v.Value.UnmarshalText([]byte(strings.TrimSpace(s)))


Suggested change

return v.Value.UnmarshalText([]byte(strings.TrimSpace(s)))

s = strings.TrimSpace(s)

I've made the necessary changes.

dearchap · 2025-02-11T14:48:01Z

flag_text_test.go

+			name: "empty",
+			flag: TextFlag{
+				Name:  "log-level",
+				Value: &slog.LevelVar{},


Note that Value is used to initialize the flag. The actual value of field should be in Destination

See the flag_generic.go as an example.

I've addressed this in the latest version of the PR.

Signed-off-by: Eng Zer Jun <[email protected]>

somebadcode · 2025-02-13T19:04:21Z

I've discovered a limitation of the way the FlagBase forces you to have T and *T. That's not suitable in this case when when it would be better for it to be encoding.TextMarshaler and encoding.TextUnmarshaler.

So the interface that I created does not equate to those two to the extent that it allows the user of the package to just put their implementation in like you can with flag.TextVar.

	text := &MockMarshaller{}

	cmd := &Command{
		Name: "foo",
		Flags: []Flag{
			&TextFlag{
				Name:        "mytext",
				Value:       text,
				Destination: &text,
			},
		},
	}

This is not valid. Because &text is of type **MockMarshaller and not *TextMarshaler. Type assertion for a simple case like this is unpleasant.

So I would either have to not use the FlagBase and re-implement everything that it gives or FlagBase must be refactored to allow for two disparate types to be used for value and destination like flag.TextVar does.

dearchap · 2025-02-13T22:02:17Z

@somebadcode can you send me the test code you are using for this ? I think it should be like this.

	text := MockMarshaller{}

	cmd := &Command{
		Name: "foo",
		Flags: []Flag{
			&TextFlag{
				Name:        "mytext",
				Value:       text,
				Destination: &text,
			},
		},
	}

somebadcode · 2025-02-13T23:35:12Z

@somebadcode can you send me the test code you are using for this ? I think it should be like this.
	text := MockMarshaller{}

	cmd := &Command{
		Name: "foo",
		Flags: []Flag{
			&TextFlag{
				Name:        "mytext",
				Value:       text,
				Destination: &text,
			},
		},
	}

Test code:

type MockMarshaller struct {
	text []byte
}

var _ encoding.TextMarshaler = (*MockMarshaller)(nil)
var _ encoding.TextUnmarshaler = (*MockMarshaller)(nil)

func (m *MockMarshaller) MarshalText() ([]byte, error) {
	return m.text, nil
}

func (m *MockMarshaller) UnmarshalText(text []byte) error {
	m.text = text
	return nil
}

func TestTextFlagValueFromCommand(t *testing.T) {
	text := &MockMarshaller{}

	cmd := &Command{
		Name: "foo",
		Flags: []Flag{
			&TextFlag{
				Name:        "mytext",
				Value:       text,
				Destination: &text,
			},
		},
	}
}

Code that I'm trying to test:

func (cmd *Command) Text(name string) TextMarshalUnmarshaler {
	if v, ok := cmd.Value(name).(TextMarshalUnmarshaler); ok {
		tracef("text available for flag name %[1]q with value=%[2]v (cmd=%[3]q)", name, v, cmd.Name)
		return v
	}

	tracef("text NOT available for flag name %[1]q (cmd=%[2]q)", name, cmd.Name)
	return nil
}

This is what I've added but not committed yet.

dearchap · 2025-02-16T19:55:19Z

@somebadcode I think the problem is you are trying to use the same instance for both Value and Destination. Generally this distinction is present when you have "simple" types

Value: 10,
Destination: &fooInt

You should test your code like this

Value: text1,
Destination: &text2,

where text1 is a pointer to marshaller/unmarshaller and text2 is a simple reference

somebadcode · 2025-02-17T08:44:41Z

@somebadcode I think the problem is you are trying to use the same instance for both Value and Destination. Generally this distinction is present when you have "simple" types
Value: 10,
Destination: &fooInt
You should test your code like this
Value: text1,
Destination: &text2,
where text1 is a pointer to marshaller/unmarshaller and text2 is a simple reference

Why would pointing back to the same one matter?
The whole issue is that it's a pointer to an interface and an interface is already a pointer to the implementation. In this case, we want to accept two distinct types for Value and Destination, encoding.TextMarshaler and encoding.TextUnmarshaler respectively, like flag.TextVar does.

In your example text1 would be a pointer to the implementation, which does satisfy the interface. We don't need to have a pointer to an interface in Destination, all we care about is that it satisfies a different interface. (see example further down)

Both Value and Destination should be plain interfaces and not pointers to interfaces.
To satisfy an interface, you must give it a pointer to the implementation after which the interface contains the pointer. So passing that interface around would be the same as passing a pointer to the implementation around. So Destination doesn't need to be a pointer and having a pointer to an interface is pointless in this case and gives us issues like this.

The type FlagBase is too strict in so far that it only works with simple types when it should work with any type with ease.

Because of the constraints of FlagBase, in Destination we must have a pointer to an interface that we're passing in as Value and to be able to do that we need to have our own interface that consists of the two interfaces that we would otherwise use for Value and Destination separately.

So FlagBase[encoding.TextMarshaler, encoding.TextUnmarshaler, TextValue] is essentially what we want.
So forcing T and *T is a huge hurdle in this case. It would be easier if we could have them completely separate and the need for TextMarshalUnmarshaler goes away and the flag would use the interfaces provided by the standard library.

Separating the two would make this implementation less complicated and those who want to add their own custom implementations for their special flags would have an easier time if it wouldn't be this constrained.

So if FlagBase is changed to this:

type FlagBase[T any, T2 any, C any, VC ValueCreator[T, T2, C]] struct {
	// ...
	Value		T	`json:"defaultValue"`
	Destination	T2	`json:"-"`
	// ...
}

This would enable us to do:

type TextFlag = FlagBase[encoding.TextMarshaler, encoding.TextUnmarshaler, StringConfig, TextValue]

This would make the implementation cleaner, more explicit and easier to use.

Simple types such as StringFlag would look like this:

type StringFlag = FlagBase[string, *string, StringConfig, StringValue]

More explicit typing reduces the constraints and increases the versatility at minimal cost (no increased runtime cost).

Look at the standard library's flag.TextVar for how it should be done when it comes to accepting interfaces for flags. The default value and the destination are two distinct interfaces, neither of them are pointers to interfaces because interfaces are already pointing to the implementations, which can be incompatible implementations.

dearchap · 2025-02-17T12:45:13Z

@somebadcode I like that idea. Do you want include the FlagBase changes in this PR ? or should I make a separate PR for that ?

somebadcode · 2025-02-17T14:51:39Z

@somebadcode I like that idea. Do you want include the FlagBase changes in this PR ? or should I make a separate PR for that ?

That would be a big change not directly related to this PR but an improvement on FlagBase itself. To make the commit history easier to follow, it should be a separate PR.

So this PR can be rebased on top of that once the changes are merged.

Also, what would the generic type be? T and T2 seems weird, but maybe T and DT or something.

dearchap · 2025-02-19T01:50:22Z

@somebadcode I started working on adding a new Destination Type and ran into some issues. I dont think its that trivial to add. I dont have cycles to spare this week so I'm not sure when I will get around to it. For now I would suggest if you use GenericValue as your base or do this particular flag as a custom flag satisfying all the necessary interfaces. When we get around to making the Destination Type change I can look into compacting TextFlag into this.

somebadcode · 2025-02-20T12:14:30Z

@somebadcode I started working on adding a new Destination Type and ran into some issues. I dont think its that trivial to add. I dont have cycles to spare this week so I'm not sure when I will get around to it. For now I would suggest if you use GenericValue as your base or do this particular flag as a custom flag satisfying all the necessary interfaces. When we get around to making the Destination Type change I can look into compacting TextFlag into this.

I tried to do the same but ran into issues with ArgumentBase. So I haven't even managed to run tests yet.
So it looks like more refactoring is needed than just changing the types. The flags that cover the simple types and slices of such types seems to work but the ArgumentBase is much more tricky.

I'll look into changing the TextFlag implementation to not use the generic structs with these limitations.

somebadcode requested a review from a team as a code owner February 10, 2025 20:01

docs: update godoc

0cd35b5

Signed-off-by: Eng Zer Jun <[email protected]>

Juneezee force-pushed the textflag branch from 1df6b84 to 0cd35b5 Compare February 11, 2025 09:50

Juneezee reviewed Feb 11, 2025

View reviewed changes

somebadcode added 4 commits February 11, 2025 13:21

fix: use stretchr/{assert,require}

215d8c7

fix: improve test coverage of TextFlag

3475c74

fix: remove underscore receiver

dd20af7

fix: remove superfluous empty line

de9fde3

dearchap reviewed Feb 11, 2025

View reviewed changes

fix: address review feedback

33d1db5

somebadcode force-pushed the textflag branch from 0876e6b to 33d1db5 Compare February 11, 2025 23:16

somebadcode added 2 commits February 12, 2025 23:50

fix: address review feedback and update doc

ff22a37

fix: adjust docs to reflect changes

35656a1

somebadcode force-pushed the textflag branch from d9552c8 to 35656a1 Compare February 12, 2025 22:56

docs: regenerate v3 godoc

9b87f3b

Signed-off-by: Eng Zer Jun <[email protected]>

Juneezee approved these changes Feb 13, 2025

View reviewed changes

dearchap self-requested a review February 16, 2025 23:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add TextFlag #2055

feat: add TextFlag #2055

somebadcode commented Feb 10, 2025 •

edited

Loading

Juneezee left a comment

Juneezee Feb 11, 2025

somebadcode Feb 11, 2025

somebadcode Feb 12, 2025

dearchap Feb 11, 2025

somebadcode Feb 12, 2025

dearchap Feb 11, 2025

somebadcode Feb 12, 2025

dearchap Feb 11, 2025

somebadcode Feb 12, 2025

dearchap Feb 11, 2025

somebadcode Feb 12, 2025

dearchap Feb 11, 2025

dearchap Feb 11, 2025

somebadcode Feb 13, 2025 •

edited

Loading

somebadcode commented Feb 13, 2025

dearchap commented Feb 13, 2025

somebadcode commented Feb 13, 2025

dearchap commented Feb 16, 2025

somebadcode commented Feb 17, 2025 •

edited

Loading

dearchap commented Feb 17, 2025 •

edited

Loading

somebadcode commented Feb 17, 2025

dearchap commented Feb 19, 2025

somebadcode commented Feb 20, 2025

-func (v TextValue) Create(t TextMarshalUnMarshaller, _ *TextMarshalUnMarshaller, c StringConfig) Value {
+func (v TextValue) Create(t TextMarshalUnMarshaller, p *TextMarshalUnMarshaller, c StringConfig) Value {
+      b, _ := p.MarshalText()
+      t.UnmarshalText(b)

	return v.Value.UnmarshalText([]byte(strings.TrimSpace(s)))
	s = strings.TrimSpace(s)

feat: add TextFlag #2055

Are you sure you want to change the base?

feat: add TextFlag #2055

Conversation

somebadcode commented Feb 10, 2025 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Release Notes

Juneezee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

somebadcode Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

somebadcode commented Feb 13, 2025

dearchap commented Feb 13, 2025

somebadcode commented Feb 13, 2025

dearchap commented Feb 16, 2025

somebadcode commented Feb 17, 2025 • edited Loading

dearchap commented Feb 17, 2025 • edited Loading

somebadcode commented Feb 17, 2025

dearchap commented Feb 19, 2025

somebadcode commented Feb 20, 2025

somebadcode commented Feb 10, 2025 •

edited

Loading

somebadcode Feb 13, 2025 •

edited

Loading

somebadcode commented Feb 17, 2025 •

edited

Loading

dearchap commented Feb 17, 2025 •

edited

Loading