-
Notifications
You must be signed in to change notification settings - Fork 71
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
Add JSON-encoded version of NULL uniqueidentifier #238
base: main
Are you sure you want to change the base?
Conversation
sedbygo
commented
Mar 4, 2025
- as literal name according RFC 7159
- remove panic: encoding/hex: invalid byte: U+006E 'n'
* as literal name according RFC 7159
@microsoft-github-policy-service agree |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #238 +/- ##
==========================================
+ Coverage 74.77% 74.91% +0.14%
==========================================
Files 32 32
Lines 6457 6462 +5
==========================================
+ Hits 4828 4841 +13
+ Misses 1338 1333 -5
+ Partials 291 288 -3 ☔ View full report in Codecov by Sentry. |
uniqueidentifier_null_test.go
Outdated
Valid: false, | ||
} | ||
|
||
b, err := json.Marshal(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.
validating Marshal
with our own Unmarshal
seems a bit underspecified. It could be writing some string that third parties don't believe represent a null value. Is that ok?
an alternative is to just verify that Marshal
writes the string "null" since the test above this one verifies that UnmarshalJSON
correctly translates "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.
Hi David, thank you for your feedback! I will make the mentioned improvement and submit it for another review.
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.
Done. Please, see the changes. Thx!
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.