Skip to content
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.

Sign up for GitHub

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

Jump to bottom

GODRIVER-2976 Remove deprecated BSON code (phase 1) #1410

Merged
merged 14 commits into from
Jan 10, 2024
Merged

GODRIVER-2976 Remove deprecated BSON code (phase 1) #1410

merged 14 commits into from
Jan 10, 2024

Conversation

qingyang-hu
Copy link
Collaborator

@qingyang-hu qingyang-hu commented Oct 4, 2023 •
edited
Loading

GODRIVER-2976

Summary

Phase 1 of the deprecation to limit the files to reviewing one time.

Background & Motivation

@mongodb-drivers-pr-bot mongodb-drivers-pr-bot
Copy link
Contributor

mongodb-drivers-pr-bot bot commented Nov 29, 2023 •
edited
Loading

API Change Report

./bson

incompatible changes

##(*Decoder).Reset: changed from func(./bson/bsonrw.ValueReader) error to func(./bson/bsonrw.ValueReader)
(Decoder).SetContext: removed
##(Decoder).SetRegistry: changed from func(./bson/bsoncodec.Registry) error to func(./bson/bsoncodec.Registry)
##(*Encoder).Reset: changed from func(./bson/bsonrw.ValueWriter) error to func(./bson/bsonrw.ValueWriter)
(Encoder).SetContext: removed
##(Encoder).SetRegistry: changed from func(./bson/bsoncodec.Registry) error to func(./bson/bsoncodec.Registry)
MarshalAppend: removed
MarshalAppendWithContext: removed
MarshalAppendWithRegistry: removed
MarshalExtJSONAppend: removed
MarshalExtJSONAppendWithContext: removed
MarshalExtJSONAppendWithRegistry: removed
MarshalExtJSONWithContext: removed
MarshalExtJSONWithRegistry: removed
MarshalValueAppend: removed
MarshalValueAppendWithContext: removed
MarshalValueAppendWithRegistry: removed
MarshalValueWithContext: removed
MarshalWithContext: removed
MarshalWithRegistry: removed
NewDecoderWithContext: removed
##NewEncoder: changed from func(./bson/bsonrw.ValueWriter) (*Encoder, error) to func(./bson/bsonrw.ValueWriter) *Encoder
NewEncoderWithContext: removed
NewFromIOReader: removed
RawValue.AsInt32: removed
RawValue.AsInt32OK: removed

@qingyang-hu qingyang-hu force-pushed the godriver2976 branch 3 times, most recently from 999bdd8 to 6409678 Compare December 5, 2023 18:14
@qingyang-hu qingyang-hu force-pushed the godriver2976 branch 2 times, most recently from 3c6dfdb to 6cc5e96 Compare December 6, 2023 19:13
@qingyang-hu qingyang-hu force-pushed the godriver2976 branch 3 times, most recently from 720f32b to 50c8cf1 Compare December 7, 2023 23:08
@qingyang-hu qingyang-hu changed the title GODRIVER-2976 Remove deprecated BSON code GODRIVER-2976 Remove deprecated BSON code (phase 1) Dec 8, 2023
@qingyang-hu qingyang-hu marked this pull request as ready for review December 8, 2023 15:55
@qingyang-hu qingyang-hu requested a review from a team as a code owner December 8, 2023 15:55
@qingyang-hu qingyang-hu requested review from blink1073 and removed request for a team December 8, 2023 15:55
@qingyang-hu qingyang-hu requested review from matthewdale and prestonvasquez and removed request for blink1073 December 8, 2023 15:56
matthewdale
matthewdale reviewed Dec 20, 2023
View reviewed changes
bson/bson_test.go Outdated
Comment on lines 181 to 182
buf := new(bytes.Buffer)
enc := new(Encoder)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's technically safe to share the buffer and Encoder in this case, but it creates an opportunity for unexpected interaction between test cases. We should initialize these values in the test case funcs.

bson/marshal_test.go Outdated
Comment on lines 30 to 31
buf := new(bytes.Buffer)
enc := new(Encoder)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should initialize these values in the test case funcs.

bson/marshal_test.go Outdated
Comment on lines 57 to 58
buf := new(bytes.Buffer)
enc := new(Encoder)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should initialize these values in the test case funcs.

mongo/options/mongooptions.go Show resolved Hide resolved
mongo/options/mongooptions.go Show resolved Hide resolved
internal/integration/unified/gridfs_bucket_operation_execution.go Outdated
@@ -148,7 +148,7 @@ func executeBucketDownloadByName(ctx context.Context, operation *operation) (*op
case "filename":
filename = val.StringValue()
case "revision":
opts.SetRevision(val.AsInt32())
opts.SetRevision(int32(val.AsInt64()))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check this conversion to make sure it doesn't overflow.

prestonvasquez
prestonvasquez previously approved these changes Jan 3, 2024
View reviewed changes
Copy link
Collaborator

@prestonvasquez prestonvasquez left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, with changes requested by @matthewdale 👍

matthewdale
matthewdale approved these changes Jan 10, 2024
View reviewed changes
Copy link
Collaborator

@matthewdale matthewdale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! 👍

@qingyang-hu qingyang-hu merged commit 8c5c0b0 into mongodb:master Jan 10, 2024
31 of 39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@prestonvasquez prestonvasquez prestonvasquez approved these changes

@matthewdale matthewdale matthewdale approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@qingyang-hu @matthewdale @prestonvasquez