-
Notifications
You must be signed in to change notification settings - Fork 9
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
A generic mechnism for supporting new type #23
Conversation
Yeah, after realizing the version change "broke" 3 or 4 of my applications I downgraded since it wasn't super important to use 1.20. I really like the concept of your latest change, so let me give some thought about going to a v2 branch or just going with 1.10+ requiring 1.20. Any suggestions on either of those? (I can "fix" my applications by telling dependabot to ignore the version change) |
if you plan to maintain both v1 and v2 branch, then create a v2 branch; otherwise just bump the minor version |
Yeah, bumping the minor version does seem best to me. 1.10.x and 1.11.0 can pick up these newest changes. |
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.
Just the one little change and it's good to go. Thanks!
simple.go
Outdated
type handlerFunc func(tag reflect.StructTag, fieldRef interface{}, | ||
hasDefaultTag bool, tagDefault string, | ||
flagSet *flag.FlagSet, renamed string, | ||
usage string, aliases string) error |
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 think it would be better to declare this in general.go
alongside supportedStructList
general.go
Outdated
// this is a list of supported struct, like time.Time, that walkFields() won't walk into, | ||
// the key is the is string returned by the getTypeName(<struct_type>), | ||
// each supported struct need to be added in this map in init() | ||
var supportedStructList = make(map[string]handlerFunc) |
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.
This needs a new name. Something like extendedTypes
?
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.
Just the one little change and it's good to go. Thanks!
done, also updated readme
after last PR, I unfortunately realized that there are a few more custom struct to support in my project, so I think there should be a mechanism for user to add support themself, and also the way last PR work could be better, so this is what this PR does, it makes supporting new type much easier
I rewrite implementation for time.Time, net.IP, net.IPNet, and net.HardwareAddr with the new way.
however this PR uses golang generic mechanism, which requires go ver1.18+, I uses 1.19 in go.mod and CI, it seems you want to use 1.16, so if you don't want take this PR, it is fine, I will just use my fork