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-1048 add default_database api for client #488

Merged
merged 9 commits into from Oct 28, 2021
Merged

RUST-1048 add default_database api for client #488

merged 9 commits into from Oct 28, 2021

Conversation

WindSoilder
Copy link
Contributor

closes: #475

Regarding to this:

A new method on Client called default_database(&self) -> Option<&str>
Though, I wonder if this would be more ergonomic if it actually returned an Option

Personally, I think make default_database(&self) returned an Option<Database> is easiler to use. And the behavior is more matches for the function name.

If user want to get default database name, we can get it from ClientOptions.default_database, or using something like client.default_database().unwrap().name().

Maybe something is missing, any review suggestion is appreciated :-)

Copy link
Contributor

@patrickfreed patrickfreed left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I have a few minor suggestions and one open question for the team, but otherwise it looks good!

src/client/options/mod.rs Show resolved Hide resolved
src/client/mod.rs Outdated Show resolved Hide resolved
src/client/mod.rs Outdated Show resolved Hide resolved
src/client/options/mod.rs Show resolved Hide resolved
src/client/options/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@patrickfreed patrickfreed left a comment

Choose a reason for hiding this comment

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

LGTM! (pending green CI run).

Thanks for your contribution!

@WindSoilder
Copy link
Contributor Author

is there any way to look into why CI failed?

Copy link
Contributor

@isabelatkinson isabelatkinson left a comment

Choose a reason for hiding this comment

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

lgtm mod a few typo fixes!

src/client/options/mod.rs Outdated Show resolved Hide resolved
src/sync/test.rs Outdated Show resolved Hide resolved
@patrickfreed
Copy link
Contributor

It looks like rustfmt needs to be run, check out the ./evergreen/check-rustfmt.sh script to see the invocation we use.

The other failures look to be some unrelated known flaky tests, so nothing to worry about there.

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! (modulo typo / rustfmt fixes)

@WindSoilder
Copy link
Contributor Author

Ok, I've fix typo and make a rebase, it should be formatted..

@patrickfreed patrickfreed merged commit 578954b into mongodb:master Oct 28, 2021
@patrickfreed
Copy link
Contributor

Thanks again for your contribution @WindSoilder!

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.

RUST-1048 support something like get_default_database from client?
4 participants