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

Add a json_pretty macro #287

Closed
wants to merge 1 commit into from
Closed

Conversation

irevoire
Copy link

Hello, I really like the crate, but when I’m embedding a lot of super small tests, it gets huge because the assert_json use the to_string_pretty function of serde_json, which takes a lot of lines for the most basics function.

I would like to have the choice to use the to_string or to_string_pretty, so here’s a first PR.
I’m afraid it’s a huge breaking, though since the assert_json_snapshot will break everywhere wdyt?

@mitsuhiko
Copy link
Owner

I don't want to change the to_json format but I'm happy to build a more compact JSON snapshot format. Do you have by any chance some reference inputs you want to snapshot so I can get an understanding of what you are trying to snapshot?

@irevoire
Copy link
Author

irevoire commented Sep 21, 2022

I don't want to change the to_json format

Honestly, I don’t think we should, but I had no idea for a new name 😂
Maybe we should call the « more compact JSON » assert_json_inlined_snapshot?

I don't want to change the to_json format but I'm happy to build a more compact JSON snapshot format. Do you have by any chance some reference inputs you want to snapshot so I can get an understanding of what you are trying to snapshot?

I don’t have much to show rn, but basically, I’m going to write a lot of tests looking like that where I try to query one document. (that were sent as JSON initially).
https://github.com/meilisearch/meilisearch/blob/35a28294313872710fa25976f7d425fe7271f4eb/index-scheduler/src/index_scheduler.rs#L450

Also, as you can see, I’m currently using another snapshot function to use a smaller debug snapshot; that’s another addition I would like to do after this one.

@mitsuhiko
Copy link
Owner

I have already been thinking in the past to add a assert_compact_json! which uses a more compact representation. I wouldn't have gone as forcing a single line and no whitespace, but to run a prettification approach where certain values are permitted to stay in a single line in collections. I can play around with this.

@irevoire
Copy link
Author

I guess it would make sense to have something like;

  • A top-level array of objects will use one line per object (look like jsonlines and is easy to diff)
  • A top-level object uses one line per field
  • Everything else is on a single line

But IMO, if I see you use serde_json, I’ll only expect you to provide the to_string and to_string_pretty methods.
And if you develop something like that, I would expect it to be available as a third option like json_smart_pretty or something 🤔

@mitsuhiko
Copy link
Owner

Since insta is no longer using serde_json I have quite some freedom to modify the serialization behavior. The current JSON serializer is in json.rs and should be easy enough to customize for different whitespace behaviors.

@mitsuhiko
Copy link
Owner

I am closing this in favor of #288.

@mitsuhiko mitsuhiko closed this Sep 22, 2022
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