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

Feature Request: Types implementing "fmt.Stringer" should be assignable/comparable to strings #648

Open
jippi opened this issue May 10, 2024 · 6 comments
Labels

Comments

@jippi
Copy link

jippi commented May 10, 2024

👋 Hello,

I got the following error with this expression: merge_request.state in ["merged", "closed"]

reflect.Value.MapIndex: value of type gitlab.MergeRequestState is not assignable to type string

It would be very nice if types implementing fmt.Stringer interface to be assignable to strings, or generally treated as strings.

Alternatively, a expr interface types could implement, making them capable of self-casting to various scalar types.

For reference this is the gitlab.MergeRequestState type (used in merge_request.state) implementation

type MergeRequestState string

const (
	// All available
	MergeRequestStateAll MergeRequestState = "all"
	// In closed state
	MergeRequestStateClosed MergeRequestState = "closed"
	// Discussion has been locked
	MergeRequestStateLocked MergeRequestState = "locked"
	// Merge request has been merged
	MergeRequestStateMerged MergeRequestState = "merged"
	// Opened merge request
	MergeRequestStateOpened MergeRequestState = "opened"
)

var AllMergeRequestState = []MergeRequestState{
	MergeRequestStateAll,
	MergeRequestStateClosed,
	MergeRequestStateLocked,
	MergeRequestStateMerged,
	MergeRequestStateOpened,
}

func (e MergeRequestState) IsValid() bool {
	switch e {
	case MergeRequestStateAll, MergeRequestStateClosed, MergeRequestStateLocked, MergeRequestStateMerged, MergeRequestStateOpened:
		return true
	}
	return false
}

func (e MergeRequestState) String() string {
	return string(e)
}
@antonmedv
Copy link
Member

Hello 👋🏻,

This is a good suggestion. I think it was already proposed multiple times. Although I believe this should not be a default behavior, we can create an official patcher (https://github.com/expr-lang/expr/tree/master/patcher) for fmt.Stringer interface.

This new patcher will automatically add .String() method calls in these cases:

  • All binary nodes:
    • foo + bar
    • foo in [x, y, z]
    • foo matches bar
  • Function calls in string params:
    • func(foo)
  • Anything else?

Also let's modify string() built in to add support for fmt.Stringer interface. We need this as in some caes Expr will bot be able to detect identifer type and users will need to manualy case to string.

Like in next situation, [foo, bar][0] has any type.

[foo, bar][0] + "sufix"

@rrb3942
Copy link
Contributor

rrb3942 commented May 15, 2024

There was some similar discussion around this for the ValueGetter patcher here: #487 (comment)

Off the top of my head time.Duration and time.Time both support fmt.Stringer, so automatic conversion might be unwanted in those cases.

@jippi
Copy link
Author

jippi commented May 15, 2024

being able to pick fmt.Stringer or expr.Stringer (as a ExprString() interface) would be neat to opt in to behavior and differentiate behaviour if desired :)

@rrb3942
Copy link
Contributor

rrb3942 commented May 16, 2024

@jippi You could take a look at https://pkg.go.dev/github.com/expr-lang/[email protected]/patcher/value and see if that does what you want (specifically implementing https://pkg.go.dev/github.com/expr-lang/[email protected]/patcher/value#StringValuer).

@jippi
Copy link
Author

jippi commented Sep 3, 2024

The valuer worked for me 👍 though it was a bit tedious having to repeat this code a handful of times

func (d MyType) AsString() string {
	return d.String()
}

That being said, I do understand why not supporting fmt.Stringer directly might be the most compatible/less-magic path - forcing engineers to decide if Expr should see something as a string or not.

Having the Valuer loaded by default (perhaps as a bool option) and clearly calling it out in the docs would have made this easier for me to action and fix out of the box :)

@antonmedv
Copy link
Member

Having the Valuer loaded by default (perhaps as a bool option) and clearly calling it out in the docs would have made this easier for me to action and fix out of the box :)

Could you tell what do you miss in the docs?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants