[RFC] many: add types.Option generic and use in customizations
#860
Conversation
mvo5
force-pushed
the
go-optional-less-ptr-internal
branch
from
e3e0606
to
ddb721c
Compare
|
|
||
| // Option is a data type that must be Some (i.e. having a value) or None (i.e. doesn't have a value). | ||
| // This type implements database/sql/driver.Valuer and database/sql.Scanner. | ||
| type Option[T OptionTomlTypes] []T |
There was a problem hiding this comment.
Fwiw, we could call this Optional too which I actually slightly prefer but then Option is what rust uses and lots of people are familar with this terminology now.
| ) | ||
|
|
||
| // Some is a function to make an Option type value with the actual value. | ||
| func Some[T OptionTomlTypes](v T) Option[T] { |
There was a problem hiding this comment.
Fwiw, we could call this differently, e.g. types.NewOptional("foo") or somesuch but again rust is the inspiration and it seems nice to keep close to their terminology as many people will be familiar with it.
There was a problem hiding this comment.
I quite like this though I don't think it gets us much benefit in Go (there's no nice syntax and stdlib to deal with options) there's also no real downside.
It's nice being able to see at the calling side of the code that we're handling an option and not a random pointer.
| @@ -39,18 +40,18 @@ func NewUsersStageOptions(userCustomizations []users.User, omitKey bool) (*Users | |||
| users := make(map[string]UsersStageOptionsUser, len(userCustomizations)) | |||
| for _, uc := range userCustomizations { | |||
| // Don't hash empty passwords, set to nil to lock account | |||
| if uc.Password != nil && len(*uc.Password) == 0 { | |||
| if len(uc.Password.Unwrap()) == 0 { | |||
There was a problem hiding this comment.
This is different behavior from the Rust we're trying to emulate where .unwrap() would panic if uc.Password is not Some.
There was a problem hiding this comment.
Yeah, I took this from the upstream go-optional lib, but happy to change and call it e.g. "UnwrapWithDefault" or "TakeWithDefault" or just "Take" to make this clearer. It convinient but I agree that for rusians it might be confusing.
There was a problem hiding this comment.
What does it do in this library if the value is None? Still panic?
This is heavily inspired by https://github.com/moznion/go-optional with is super nice. However we cannot use it as is because it lacks support for toml unmarshal and go makes it really hard to extend existing types. Submiting toml unmarshal upstream would also be an option but it's difficult in the general case because the toml unmarshal interface is very limited for complex types (it only passes `data any` with the full decoded complex struct into map[string]inteface{} for example which leaves it to the user of the library to convert to the actual type struct which is a pain with reflect). I really whish go was more expressive here. So this commit defines a subset of https://github.com/moznion/go-optional in our own internal types lib.
It seems we are currently using *bool a lot to distinguish between
{unset,true,false}. This commit instead uses the new `interal/types`
library to distinguish between unset/undefined and the value.
It uses generics to works on all primitive toml type.
mvo5
force-pushed
the
go-optional-less-ptr-internal
branch
from
23e0f8b
to
07741d4
Compare
|
This PR is stale because it has been open 30 days with no activity. Remove "Stale" label or comment or this will be closed in 7 days. |
|
This PR is stale because it has been open 30 days with no activity. Remove "Stale" label or comment or this will be closed in 7 days. |
For customizations we currently use pointers when we want to distinguish between set or unset value. This works fine and is supported by json/toml marshaling natively. However we lose some safety because there is always the risk of dereferencing a pointer and extra checks for
nil.This PR introduces a new
types.Optionthat wraps the primitive types bool,int,string [0] so that they are there is a distinction between "set" (Some) and "unset" (None) [1]. This allows us to do things like:or
And there is overall a much smaller risk of making mistakes while also be more explicit (well, minor as arguably is as
*is also a strong indicator).This PR converts a small subset to show what it would look like, it's a lot of busywork so maybe not worth the effort but I like it over the pointers (but I understand if other wouldn't).
[0] Sadly the go type system and toml make it hard to do the same for structs which would be extremely nice, see the commit message/code comments for details.
[1] Of course this is very much inspired by the rust std::Some type and the go-optional libraries.