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.

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

GODRIVER-2962 Remove setters from Aggregate. #1372

Conversation

matthewdale
Copy link
Collaborator

GODRIVER-2962

Summary

Remove setters from Aggregate.

Background & Motivation

The field setters on the operation types in the x/mongo/driver/operation package provide no usability improvement but create a maintenance burden whenever we have to update the operations. Remove the field setters and use struct literals instead.

Split the change into multiple PRs to make reviewing them easier.

@matthewdale matthewdale requested a review from a team as a code owner September 1, 2023 19:22
@matthewdale matthewdale requested review from blink1073 and removed request for a team September 1, 2023 19:22
@matthewdale matthewdale temporarily deployed to api-report September 1, 2023 19:22 — with GitHub Actions Inactive
@github-actions
Copy link

github-actions bot commented Sep 1, 2023

API Change Report

./x/mongo/driver/operation

incompatible changes

(*Aggregate).AllowDiskUse: removed
(*Aggregate).BatchSize: removed
(*Aggregate).BypassDocumentValidation: removed
(*Aggregate).ClusterClock: removed
(*Aggregate).Collation: removed
(*Aggregate).Collection: removed
(*Aggregate).CommandMonitor: removed
(*Aggregate).Comment: removed
(*Aggregate).Crypt: removed
(*Aggregate).CustomOptions: removed
(*Aggregate).Database: removed
(*Aggregate).Deployment: removed
(*Aggregate).HasOutputStage: removed
(*Aggregate).Hint: removed
(*Aggregate).Let: removed
(*Aggregate).MaxTime: removed
(*Aggregate).Pipeline: removed
(*Aggregate).ReadConcern: removed
(*Aggregate).ReadPreference: removed
(*Aggregate).Retry: removed
(*Aggregate).ServerAPI: removed
(*Aggregate).ServerSelector: removed
(*Aggregate).Session: removed
(*Aggregate).Timeout: removed
(*Aggregate).WriteConcern: removed
NewAggregate: removed

compatible changes

Aggregate.AllowDiskUse: added
Aggregate.BatchSize: added
Aggregate.BypassDocumentValidation: added
Aggregate.Clock: added
Aggregate.Collation: added
Aggregate.Collection: added
Aggregate.Comment: added
Aggregate.Crypt: added
Aggregate.CustomOptions: added
Aggregate.Database: added
Aggregate.Deployment: added
Aggregate.HasOutputStage: added
Aggregate.Hint: added
Aggregate.Let: added
Aggregate.MaxTime: added
Aggregate.Monitor: added
Aggregate.Pipeline: added
Aggregate.ReadConcern: added
Aggregate.ReadPreference: added
Aggregate.Retry: added
Aggregate.Selector: added
Aggregate.ServerAPI: added
Aggregate.Session: added
Aggregate.Timeout: added
Aggregate.WriteConcern: added

blink1073
blink1073 previously approved these changes Sep 5, 2023
Copy link
Member

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

LGTM!

@prestonvasquez prestonvasquez changed the base branch from master to v1 September 13, 2023 18:05
@matthewdale matthewdale changed the base branch from v1 to master September 20, 2023 04:38
@matthewdale matthewdale dismissed blink1073’s stale review September 20, 2023 04:38

The base branch was changed.

@blink1073
Copy link
Member

Is this meant to be part of the 2.0 epic then?

@matthewdale
Copy link
Collaborator Author

@blink1073 no, this is not a strictly v2 change. I switched the branch to minimize the additional work required to sync changes from v1 to master because it isn't required in v1.

@blink1073
Copy link
Member

Should we mark the setters as deprecated in v1?

@matthewdale
Copy link
Collaborator Author

@blink1073 Yes. Considering the feedback we've received about the unannounced removal of some other experimental APIs, I think marking those as deprecated in v1.x would be useful messaging.

@matthewdale
Copy link
Collaborator Author

Closing this for now, will follow up later when GODRIVER-2962 is scheduled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants