Skip to content

RUST-282 Add pretty-printed Debug implementation to Document #262

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

Merged
merged 4 commits into from
Jun 9, 2021

Conversation

NBSquare
Copy link
Contributor

@NBSquare NBSquare commented Jun 4, 2021

It looks like the only relevant struct which doesn't derive Debug is Document, so I added support for pretty-print there.

I also added a test to the end of bson.rs test.

@NBSquare NBSquare requested a review from isabelatkinson June 4, 2021 21:08
@NBSquare NBSquare changed the title RUST-310 Add pretty-printed Debug implementation to Document RUST-282 Add pretty-printed Debug implementation to Document Jun 7, 2021
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.

this looks great, just one small formatting request

@NBSquare NBSquare requested a review from isabelatkinson June 7, 2021 15:34
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! Tagging in Patrick for review.

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.

Looks good! I just have a test suggestion and request for an existing but related issue to be fixed.

@NBSquare
Copy link
Contributor Author

NBSquare commented Jun 8, 2021

I modified the Debug implementation for ObjectId to reflect the fact that ObjectId is basically a tuple wrapping the ID slice, which meant modified a unit test or two. Check that out here.

This means that instead of printing out ObjectIds as

ObjectId(000000000000000000000000)

it will print them out as

ObjectId("000000000000000000000000")

or

ObjectId(
    "000000000000000000000000",
)

depending on the format specifier. This is an easy fix if we want to follow the pattern serde_json uses for Objects and Arrays.

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! Great job with this, especially with handling all the more ambiguous cases.

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.

nice job!

@NBSquare NBSquare merged commit 8eed38f into mongodb:master Jun 9, 2021
@NBSquare NBSquare deleted the RUST-282 branch June 15, 2021 13:28
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