-
Notifications
You must be signed in to change notification settings - Fork 673
NOISSUE - Update nullable handling #2999
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2999 +/- ##
==========================================
+ Coverage 23.69% 24.83% +1.13%
==========================================
Files 360 124 -236
Lines 67764 36646 -31118
==========================================
- Hits 16058 9100 -6958
+ Misses 50922 27093 -23829
+ Partials 784 453 -331 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| Value T | ||
| } | ||
|
|
||
| // FromString[T any] represents a parser function. It is used to avoid | ||
| func New[T any](v T, set bool) Value[T] { | ||
| return Value[T]{ | ||
| Set: set, | ||
| Value: v, | ||
| } | ||
| } | ||
|
|
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.
What do you think about below approach ?
| Value T | |
| } | |
| // FromString[T any] represents a parser function. It is used to avoid | |
| func New[T any](v T, set bool) Value[T] { | |
| return Value[T]{ | |
| Set: set, | |
| Value: v, | |
| } | |
| } | |
| type Nullable interface { | |
| ~string | ~int | ~uint | |
| } | |
| // Value type is used to represent difference betweeen an | |
| // intentionally omitted value and default type value. | |
| type Value[T Nullable] struct { | |
| Set bool | |
| Value T | |
| } | |
| func New[T Nullable](v T) Value[T] { | |
| return NewNull[T].Set(v) | |
| } | |
| func NewNull[T Nullable]() Value[T] { | |
| return Value[T]{} | |
| } | |
| // Set sets the value and marks it as set. | |
| func (v *Value[T]) Set(val T) { | |
| v.Set = true | |
| v.Value = val | |
| } | |
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.
If T is any we could not determine v == nil
func New[T](v T) Value[T] {
if v == nil { // x not possible
...
}If we have constraint like below, then it makes sure that pointer or interface can't be passed so New[T Nullable] function can set without checking for nil
type Nullable interface {
~string | ~int | ~uint
}func New[T Nullable](v T) Value[T] {
return NewNull[T].Set(v)
}
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 have added a union, but let's keep the factory method signature as is - it's simple enough and works for all cases. Setting the value is a nice idea, but we'd have to make it non-exported and use getters and setter, which is not Go idiomatic, and also does not bring anything to the table unless we make both set and value non-exported. Then, we can enforce that Set always makes set true, but I'm not sure that's even desired behavior. New(value, bool) should be enough and it's also the most flexible.
internal/nullable/value.go
Outdated
| } | ||
|
|
||
| // FromString[T any] represents a parser function. It is used to avoid | ||
| func New[T nullable](v T, set bool) Value[T] { |
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.
If we have nullable, then value could not be null
example:
var a int = 0
Then the following is not true
New(a,false)
a is not null, I see Set is used for to determine any value is set or value is nil,
by default int goes to 0 which is not nil
So new function can be like New(a)
If some one need to have int with null, then we can provide NewNull[int]()
a := NewNull[int]()
or
var a Value[int] which defaults to null
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.
It's OK to restrict null value, that's why we have the Set field. If Set is false, we don't care about value. If it's true, we take the value, even if default.
pkg/sdk/groups_test.go
Outdated
| group.ID = generateUUID(t) | ||
|
|
||
| updatedDesc := nullable.Value[string]{Set: true, Value: updatedDescription} | ||
| updatedDesc := nullable.New(updatedDescription, true) |
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.
Please update this function
2f5a00d to
f7b6f96
Compare
Signed-off-by: Dusan Borovcanin <[email protected]>
Signed-off-by: Dusan Borovcanin <[email protected]>
Signed-off-by: Dusan Borovcanin <[email protected]>
Signed-off-by: Dusan Borovcanin <[email protected]>
Signed-off-by: Dusan Borovcanin <[email protected]>
Signed-off-by: Dusan Borovcanin <[email protected]>
Signed-off-by: Dusan Borovcanin <[email protected]>
Signed-off-by: Dusan Borovcanin <[email protected]>
Signed-off-by: Dusan Borovcanin <[email protected]>
f7b6f96 to
9eacb42
Compare
Signed-off-by: Dusan Borovcanin <[email protected]>
Signed-off-by: Dusan Borovcanin <[email protected]>
What type of PR is this?
This is a refactor.
What does this do?
This changes the Nullable parser naming and adds a factory method to return a valid nullable with value.
Which issue(s) does this PR fix/relate to?
There is no such issue.
Have you included tests for your changes?
Yes.
Did you document any new/modified feature?
N/A
Notes
N/A