-
-
Notifications
You must be signed in to change notification settings - Fork 72
Implement pretty_format_json as builtin hook
#915
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
base: master
Are you sure you want to change the base?
Conversation
897d3f3 to
ae79e95
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #915 +/- ##
==========================================
+ Coverage 90.06% 90.45% +0.39%
==========================================
Files 65 66 +1
Lines 12077 12575 +498
==========================================
+ Hits 10877 11375 +498
Misses 1200 1200 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ae79e95 to
386ebd2
Compare
|
Not sure why our binary sizes don't match, a good idea would be to have this computed on CI (I know CodeCov can do that automatically for JavaScript bundles or something? But not sure about Rust) Will open an issue about that, but relative size like you did is fine 👍 Edit: oh, did you build in release mode? I doubt it would matter though I thought similar was the same crate difftastic uses but that is strsim It says there is a testing crate for it https://github.com/mitsuhiko/similar-asserts maybe that could simplify the tests, but no need to |
|
@lmmx Yes I did, assumed you did aswell fom this snippet: |
|
@lmmx I'll take a look at simplifying the tests. Could also assign a new issue to me, so I can pick it up later on, when time allows |
All good! Just FYI, you don't need to be assigned anything to work on it here. Open source repos work on a pull-based model where contributors pick up issues they want to work on. I'm just another contributor here, not a maintainer, so I can't assign tickets anyway 😁 My comment was just a suggestion if you were after any more ideas, feel free to leave it as is. Enjoy your weekend 😄 |
I think you might be on different operating systems or architectures? |
|
Arm, macbook here! Didnt think it would be such a difference. Like the ci suggestion! |
Linux, no I didn't expect it to be such a gap either |
pretty_format_json as builtin hook
386ebd2 to
333ae62
Compare
333ae62 to
107da9e
Compare
📦 Cargo Bloat ComparisonBinary size change: +0.62% (16.1 MiB → 16.2 MiB) Expand for cargo-bloat outputPR Branch ResultsBase Branch Results |
|
@j178 Anything I can add to this PR? I checked the similar-asserts crate, but didn't find it cleaned up tests more than simple test files, like it has at the moment. If you still prefer the other way, I'm happy to change it. |
|
Your work is fantastic - the rest is on my end now. I just need more time to figure out all the details and make sure everything lines up with the Python implementation. If I think anything needs tweaking, I’ll make adjustments based on what you’ve built. Thanks for all your hard work! |
Description
Following the information provided in 880. This is an implementation of the
pretty-format-jsonhook.The pretty-format-json has 1k grep.app hits. It's the final unimplemented hook used by
airflowas mentioned in this comment.Notes
Preserve JSON Order
preserve_orderfeature toserde_json. This preventsserdefrom automatically ordering JSON data, which is important for comparisons with older implementations.serdeuses aBTreeMapforObject. Enablingpreserve_orderswitches it toIndexMap.sort-keysargument.Git-Style Diff with
similarsimilarlibrary to perform git-style diff checks.pre-commit.Q: why is my size so much smaller than both sizes mentioned here?
Performance
Ran these commands on the
airflowrepository:Output
Old pre-commit:

New:

Where both would autofix to the same:
