-
Notifications
You must be signed in to change notification settings - Fork 755
Nil ptr is nullable #661
base: master
Are you sure you want to change the base?
Nil ptr is nullable #661
Conversation
Codecov Report
@@ Coverage Diff @@
## master #661 +/- ##
==========================================
+ Coverage 48.3% 48.42% +0.12%
==========================================
Files 34 34
Lines 8378 8386 +8
==========================================
+ Hits 4047 4061 +14
+ Misses 3844 3838 -6
Partials 487 487
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #661 +/- ##
==========================================
+ Coverage 48.3% 48.42% +0.12%
==========================================
Files 34 34
Lines 8378 8386 +8
==========================================
+ Hits 4047 4061 +14
+ Misses 3844 3838 -6
Partials 487 487
Continue to review full report at Codecov.
|
|
||
_, err := testEngine.Insert(c) | ||
_, err := testEngine.Nullable("nullable2").Insert(c) |
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.
so maybe add another insert to remove Nullable?
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.
do you agree that Nullable() is not need here?
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.
Yes, since there is a xorm:"null"
and it's really a nil pointer. I think we should give it a null on DB as default. Any idea?
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.
let me see
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 don't like the idea with session.Statement.nullableMap
. Looks like it can be filled only from .Sync(c)
. I`m not sure is all use this method for migration.
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.
So what the default value do you think if have or not .Nullable("nullable2")
?
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.
currently, we have a empty string but I would like to have Nil
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.
@lunny, what is your answer?
please resolve conflicts. |
test for this pull request #531