Added XML support for NullDecimal #192
Conversation
bharath23
commented
Dec 4, 2020
bharath23
commented
- Added UnmarshalText and MarshalText support for NullDecimal
- Add tests for XML operations on NullDecimal
| if str == "" { | ||
| d.Valid = false | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Is this case necessary? Wouldn't the below if statement be enough?
There was a problem hiding this comment.
Decimal.Unmarshal returns error. For NullDecimal it is not an error as NullDecimal.Valid will be set to false.
https://play.golang.org/p/YJWM2pykj75
Also, we cannot assume that if its an errors its null. There could be other reasons why Decimal.Unmarshal could fail.
There was a problem hiding this comment.
It is similar to how NullDecimal.UnmarshalJSON works.
There was a problem hiding this comment.
Ok, got it :3 Thanks!
I will get back to your PR tomorrow if you don't mind :) I would love to check the unit test too
There was a problem hiding this comment.
I'm so sorry :<. I completely forgot about your pull request Bharath. I'm leaving in about an hour for holiday dinner with family, but I will check those unit test tomorrow.
I'm adding this to my TODO list, so I will not forget about it :D
| func (d *NullDecimal) UnmarshalText(text []byte) error { | ||
| str := string(text) | ||
|
|
||
| if str == "" { |
There was a problem hiding this comment.
So this is basically the case for empty XMLs and XMLs withotu the body e.g <test></test>?
Could you add a short comment that would denote that, so it will be easier to refactor in the future if needed? :D
decimal.go
Outdated
| if err := d.Decimal.UnmarshalText(text); err != nil { | ||
| return err |
There was a problem hiding this comment.
Shouldn't we explicitly set here d.Valid = false?
There was a problem hiding this comment.
We are returning error. d is invalid and shouldn't be used by caller. We can set it to false, I dont have strong opinion about it.
| @@ -766,6 +766,75 @@ func TestBadXML(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func TestNullDecimalXML(t *testing.T) { | |||
There was a problem hiding this comment.
I see that you've based those unit test on NullDecimal JSON tests and there is 3 uni test combined into one. It's not a bad thing in my opinion, but could you separate them by adding a one-liner comment before each section? So next contributors would know what test cases are covered here :3
* Added UnmarshalText and MarshalText support for NullDecimal * Add tests for XML operations on NullDecimal
bharath23
force-pushed
the
nulldecimal.XMLSupport
branch
from
f74a52b to
d05bf85
Compare
