add: MarshalJSON methods for *Array and *Document #70
Conversation
Signed-off-by: krishna sindhur <[email protected]>
Signed-off-by: krishna sindhur <[email protected]>
There was a problem hiding this comment.
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (4)
wirebson/document_test.go:54
- Consider adding a test case for marshalling an empty Document to ensure it marshals correctly.
t.Run("MarshalJSON_PreserveOrder", func(t *testing.T) {
wirebson/document.go:261
- The error message 'nil Document' is unclear. It should be more descriptive, such as 'Document is nil and cannot be marshaled to JSON'.
return nil, lazyerrors.Errorf("nil Document")
wirebson/document.go:259
- The manual construction of the JSON object in 'Document.MarshalJSON' is error-prone. Consider using 'json.Marshal' for the entire 'doc.fields' slice to simplify the implementation and avoid potential issues.
func (doc *Document) MarshalJSON() ([]byte, error) {
wirebson/array.go:185
- [nitpick] The error message 'nil Array' is unclear. Consider changing it to 'Cannot marshal a nil Array'.
return nil, lazyerrors.Errorf("nil Array")
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #70 +/- ##
==========================================
- Coverage 63.03% 62.96% -0.07%
==========================================
Files 41 41
Lines 2221 2252 +31
==========================================
+ Hits 1400 1418 +18
- Misses 645 654 +9
- Partials 176 180 +4
... and 1 file with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. |
wirebson/array_test.go
Outdated
There was a problem hiding this comment.
Please update tests, benchmarks, and fuzzing in bson_test.go instead
There was a problem hiding this comment.
Please don't resolve conversations. Please also update existing tests/benchmarks/fuzzes, not add new ones.
wirebson/document.go
Outdated
| jsonObject := make([]byte, 0) | ||
| jsonObject = append(jsonObject, '{') |
There was a problem hiding this comment.
| jsonObject := make([]byte, 0) | |
| jsonObject = append(jsonObject, '{') | |
| res := []byte{'{'} |
There was a problem hiding this comment.
@AlekSi any changes required here? I have updated to must.NotBeZero(doc)
There was a problem hiding this comment.
I'm not sure why this conversation was resolved without changes applied
There was a problem hiding this comment.
I have not added test for error scenarios In MarshalJSON is it ok? Because I don't see any error scenarios tests added for encode functions also!! Please correct me if I am wrong. If necessary I can add test for error scenarios.
There was a problem hiding this comment.
This comment does not belong to this conversation, where I asked you to replace make+append combo with a single assignment, and even provided a suggested change
There was a problem hiding this comment.
Sorry but I can't see your comments you added to replace make+append to single line, Which ended with some confusions.
Signed-off-by: krishna sindhur <[email protected]>
wirebson/bson_test.go
Outdated
| @@ -788,6 +789,30 @@ func TestNormal(t *testing.T) { | |||
| require.NoError(t, err) | |||
| assert.Equal(t, tc.raw, raw) | |||
| }) | |||
|
|
|||
| t.Run("ArrayMarshalJSON", func(t *testing.T) { | |||
There was a problem hiding this comment.
There is no point in running those subtests for each tc with the same input and expected output. Instead, all subtests use tc.
Please slow down, check all tests in this file, and make sure your changes follow its structure.
wirebson/bson_test.go
Outdated
| @@ -741,6 +742,27 @@ var decodeTestCases = []decodeTestCase{ | |||
| }, | |||
| } | |||
|
|
|||
| var marshalJSONTestCases = []normalTestCase{ | |||
There was a problem hiding this comment.
Please update existing normalTestCases variable instead
Closes #49