-
Notifications
You must be signed in to change notification settings - Fork 0
feat: unkown case #1
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
base: master
Are you sure you want to change the base?
Conversation
benjajaja
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would just like to be able to set the default value, and also the suffix: we use "not_set" or similar in our codebase, but also probably not 100% consistently.
Also in general I would be inclined to call it "default" instead of unknown.
| lowercaseLookup bool | ||
| caseInsensitive bool | ||
| marshal bool | ||
| unknownCase bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| unknownCase bool | |
| unknownCase string | |
| unknownCaseValue string | |
| unknownCaseSuffix string |
I would make this as customizable as possible here. Allow setting a default value, and the suffix. For example in re:cap BE, the value is usually NOT_SET.
(We can't just use unknownCase string, in case someone wants to use an empty string as default).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about this, but was wondering how to have a different syntax between renaming and setting the default case:
type Gender string
// This renames between type and value:
// ENUM(male, female, unknown=not_set)
// Maybe something like this?
// ENUM(male, female) default=not_setThis is a bit more work because we need to have extra parsing but doable.
What difference do you make between unknownCase and unknownCaseValue? And by suffix you mean the naming of the default type value right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could just add it to the CLI args for now.
unknownCase:bool-> enable if trueunknownCaseValue:*string-> set default value if not nilunknownCaseSuffix:*string-> replace theUnknownsuffix if not nil
Actually, the args would have to be *string, as "" would be a valid use case for the value, and unknownCaseSuffix is optional.
I'm not sure if "suffix" is actually needed, what we need (would be great) to make work is our use case that we have elsewhere:
// ENUM(first_item)
// should produce:
const OurEnumFirstItem OutEnum = "first_item"
const OurEnumNotSet OutEnum = "NOT_SET"mixed upper/lower, and snake_case for the values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But anyway don't let me drag this out too much, we can also change this later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CLI args apply for all enums defined in the file though, so I think if we have customization of the value and suffix, I think we should do it at the enum level 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, that way it's more flexible and the //go generate ... trigger can be the same across files 👍
|
Overall this looks great! Looking forward to using it elsewhere in our codebase. |
No description provided.