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-2019 Respect pretty printing flag for Document and Bson #501

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jadalilleboe
Copy link

RUST-2019

Didn't add a unit test because Display outputs to the console, let me know if this should be tested though
I did test manually with this code

let my_hashmap = HashMap::from([
        ("hello", "world!"), ("world", "hello"), ("key", "val")
      ]);
      
    let bson_document = to_bson(&my_hashmap).unwrap();
    println!("{bson_document}");
    println!("{bson_document:#}");

    let hm: HashMap<&str, &str> = HashMap::from([]);
    let md = to_document(&hm).unwrap();
    println!("{md}");
    println!("{md:#}");

which outputted

{ "hello": "world!", "world": "hello", "key": "val" }
{
 "hello": "world!", 
 "world": "hello", 
 "key": "val"
}
{}
{}

did the same test switching to_document instead of to_bson and got the same output.

@abr-egn
Copy link
Contributor

abr-egn commented Oct 17, 2024

You'll need some additional logic in there to handle nested documents, those should have the flag passed in and then be indented for the nesting.

Didn't add a unit test because Display outputs to the console, let me know if this should be tested though

I do think these need some tests. Happily, Display and Debug are actually agnostic to where they're being written to :) You can use the format! macro to get a String output. Also, you can construct a BSON Document more conveniently with the doc! macro.

As a side note, we usually do development on our own forks of the repository so the main repo doesn't accumulate dev branches (and to make accidental main-branch-clobbering less likely, not that yours truly would have done that on his first day). No need to switch for this PR, just to know for the future.

@jadalilleboe
Copy link
Author

@abr-egn Added some logic for nested documents and added some tests, let me know what you think. also thought it would be good to add some logics for arrays as well:

{
 "hello": [
  1, 
  2, 
  3
 ]
}

noted about the forks, will adhere to that in the future

@abr-egn
Copy link
Contributor

abr-egn commented Oct 21, 2024

Good thought on the array formatting!

It looks like there's some extra spacing on initial indented lines - I tried this test:

#[test]
fn test_pretty_printing() {
    let d = doc! { "hello": "world!", "world": "hello", "key": "val" };
    let expected = r#"{ "hello": "world!", "world": "hello", "key": "val" }"#;
    let formatted = format!("{d}");
    assert_eq!(
        expected, formatted,
        "expected:\n{expected}\ngot:\n{formatted}"
    );

    let d = doc! { "hello": "world!", "nested": { "key": "val", "double": { "a": "thing" } } };
    let expected = r#"{
 "hello": "world",
 "nested": {
  "key": "val",
  "double": {
   "a": "thing"
  }
 }
}"#;
    let formatted = format!("{d:#}");
    assert_eq!(
        expected, formatted,
        "expected:\n{expected}\ngot:\n{formatted}"
    );
}

and the output was

assertion `left == right` failed: expected:
{
 "hello": "world",
 "nested": {
  "key": "val",
  "double": {
   "a": "thing"
  }
 }
}
got:
{
 "hello": "world!", 
 "nested": {
   "key": "val", 
  "double": {
     "a": "thing"
  }
 }
}
  left: "{\n \"hello\": \"world\",\n \"nested\": {\n  \"key\": \"val\",\n  \"double\": {\n   \"a\": \"thing\"\n  }\n }\n}"
 right: "{\n \"hello\": \"world!\", \n \"nested\": {\n   \"key\": \"val\", \n  \"double\": {\n     \"a\": \"thing\"\n  }\n }\n}"

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