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

Allow string enums to use set string values in stringer function #8

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lucvankessel
Copy link

@lucvankessel lucvankessel commented Nov 5, 2023

This MR is related to #7
This makes it so that if a enum is of type string it will use the given values in the enum definition.

type Day string

const (
    Unknown Day = "Uknown" //enum:invalid
    Monday  Day = "MonDay"
    Tuesday Day = "Tuesday"
    Wednesday Day = "wednesDay"
    Thursday Day = "Thursday"
    Friday Day = "Friday"
    Saturday Day = "Saturday"
    Sunday Day = "sunday!"
)

This will become the following stringer function

func (day_enum Day) String() string {
    switch day_enum {
    case Unknown:
        return "Uknown" 
    case Monday:
        return "MonDay" 
    case Tuesday:
        return "Tuesday" 
    case Wednesday:
        return "wednesDay" 
    case Thursday:
        return "Thursday" 
    case Friday:
        return "Friday" 
    case Saturday:
        return "Saturday" 
    case Sunday:
        return "sunday!" 
    default:
        panic(fmt.Sprintf("no default for enum %T, invalid value: '%#v'", day_enum, day_enum))
    }
}

There is one issue that i found could happen, namely that if someone did not fill in all string values in the enum like so:

type Test string

const (
    Unknown Test = "Uknown" //enum:invalid
    Value1  Test = "test1"
    Value2 Test = "test2"
    Value3
)

It would cause a error in the switch case. thats why i added a loop to check for unique values. in general i think this is good to have to prevent duplicate values in enums because i would say enum values should be unique.
Let me know if things need added or changed.

@olistrik
Copy link
Contributor

olistrik commented Nov 6, 2023

@lucvankessel enum values not being unique is a fairly common usecase to provide aliases:

const (
  Monday Day = iota
  Tuesday
  Wednesday
)

const (
  mandag Day = iota
  dinsdag
  woensdag
)

@Daan-Adrichem and I had a brief discussion about this issue the other day. Maybe we can continue that here.

I personally don't have an issue with this approach as I cannot see a use case for having a stringly typed enum with generated enum keys. I would however argue that maybe we want to avoid implementing a stringer on stringly typed enums at all.
So rather than calling .String() directly, we use fmt.Sprint to format the enums.

@lucvankessel
Copy link
Author

lucvankessel commented Nov 6, 2023

@olistrik To support aliases we do need to make a change however. This is due to the following error "duplicate case mandag (constant 0 of type Day) in expression switch" occuring, this is the same error i had with the strings and tried to fix it with the unique loop.
Maybe changing from using switch cases to if statements? I do not see how we could work around this while still using switches. but please prove me wrong if there is a way 😅

My main use case for this would be to provide an enum with values that do not follow a strict case type (for example the enum definition above). this issue occured to me when having to implement an api which has an enum but with values that are not consistent. i want to replicate this api with these values to use in a gql input and in requests to said api.

@dadrichem dadrichem self-assigned this Nov 6, 2023
@dadrichem
Copy link
Contributor

@lucvankessel @olistrik the point about go-enum is to decouple internal and external values. Using the internal value as the external value removes this abstraction. Where I agree with the sentiment, I disagree with always coupling string enums to their internal values. Adding a flag could do the trick, I have made such a pull-request before. That was limited to some of the marshallers which work without specific string layouts (if I remember correctly everything except ent). That could be extended for string values to include ent. Personally I would suggest to use go-enum to setup basic data validation but create a single marshaller that allows these odd-ball strings. That way the developer (in this case @lucvankessel ) does not wast any time but it does not influence the entire enum.

@olistrik
Copy link
Contributor

olistrik commented Nov 6, 2023

Maybe changing from using switch cases to if statements?

@lucvankessel I think making the switch generation smart enough to not include duplicates would also be sufficient.

@dadrichem this isn't related to ent. We're communicating with another API over REST that has inconsistent casing in their own enums.

As this is purely a mapping of an inconsistent enum that is outside our control, I'd argue that any generation of keys is undesirable. Which is why I suggested perhaps not generating a stringer interface and using Sprint Vs .String() directly.

There's two ways we can facilitate this;

  1. a flag such as -case=custom
  2. Not generating a stringer for already string enums.

I personally lean towards 2 because I can't think of a use case where this behaviour is desirable by default:

const (
  Monday Day = "Maandag"
)

var foo Day = Monday
var bar string = "Monday"

fmt.Printf("%s == %s: %t\n", foo, bar, foo == bar)
// Monday == Monday: false

I guess we could also do both, provide a -case=custom, and default to that case for string enums.

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.

3 participants