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

RUST-2097 Migrate from derivative to derive-where #1245

Merged
merged 3 commits into from
Nov 13, 2024

Conversation

isabelatkinson
Copy link
Contributor

@isabelatkinson isabelatkinson commented Nov 12, 2024

Migrates from derivative to derive-where. In the long term, I'd like to switch over to derive_more, which we already depend on for some Display-related utilities. derive_more has the largest feature set of crates of its ilk; however, its MSRV for the features we'd need is 1.75 (compared to our current 1.67), and it does not yet support PartialEq. We can make the swap once those concerns are resolved. This is tracked in RUST-2102.

@isabelatkinson isabelatkinson marked this pull request as ready for review November 13, 2024 16:01
crypt: Crypt,
exec: CryptExecutor,
internal_client: Option<Client>,
_internal_client: Option<Client>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got a dead code lint error for this field when I switched the Debug impl on the struct, but deleting it caused errors 🤔 I think something else is holding a weak reference to it?

Copy link
Contributor

@abr-egn abr-egn Nov 13, 2024

Choose a reason for hiding this comment

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

The CryptExecutor holds a WeakClient, so that's probably it. I vaguely recall that it's weak because whether it's using internal_client or a user-supplied Client depends on whether the context is implicit or explicit encryption, and if it's user-supplied we don't want to hold a strong reference and keep it alive longer than the user expects.

Copy link
Contributor

@abr-egn abr-egn left a comment

Choose a reason for hiding this comment

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

LGTM!

crypt: Crypt,
exec: CryptExecutor,
internal_client: Option<Client>,
_internal_client: Option<Client>,
Copy link
Contributor

@abr-egn abr-egn Nov 13, 2024

Choose a reason for hiding this comment

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

The CryptExecutor holds a WeakClient, so that's probably it. I vaguely recall that it's weak because whether it's using internal_client or a user-supplied Client depends on whether the context is implicit or explicit encryption, and if it's user-supplied we don't want to hold a strong reference and keep it alive longer than the user expects.

@abr-egn
Copy link
Contributor

abr-egn commented Nov 13, 2024

In the long term, I'd like to switch over to derive_more, which we already depend on for some Display-related utilities. derive_more has the largest feature set of crates of its ilk; however, its MSRV for the features we'd need is 1.75 (compared to our current 1.67), and it does not yet support PartialEq. We can make the swap once those concerns are resolved.

Can you file a ticket so we remember to do this?

@isabelatkinson isabelatkinson changed the title RUST-1454 Migrate from derivative to derive-where RUST-2097 Migrate from derivative to derive-where Nov 13, 2024
@isabelatkinson isabelatkinson merged commit 80bab06 into mongodb:main Nov 13, 2024
17 checks passed
@isabelatkinson isabelatkinson deleted the derive-where branch December 6, 2024 17:03
@TroyKomodo
Copy link

Can we make a minor release of this 3.1.1 on crates.io @isabelatkinson

isabelatkinson added a commit to isabelatkinson/mongo-rust-driver that referenced this pull request Dec 10, 2024
@isabelatkinson
Copy link
Contributor Author

@TroyKomodo 3.1.1 was just released!

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.

3 participants