hirosystems / stacks.js

JavaScript libraries for identity, auth, storage and transactions on the Stacks blockchain.
https://stacks.js.org
MIT License
949 stars 309 forks source link

feat: sort tuple properties in cl pretty print #1631

Closed hugocaillard closed 7 months ago

hugocaillard commented 7 months ago

This PR was published to npm with the version 6.11.4-pr.3d18e85.0 e.g. npm install @stacks/common@6.11.4-pr.3d18e85.0 --save-exact

Description

Change Cl.prettyPrint so that it sorts properties in a tuple. This an opinionated change and I could do it in Clarinet, but it kind of make sense to have it natively here as well. I would understand if one disagrees though.

Breaking change?

Not really, in the sense that developers shouldn't rely on the property output of this method, this is mostly for debugging

Example

Cl.prettyPrint is used in the clarinet sdk to show output diffs in tests. Because the output is just a string, Vitest can fail to properly render the diff of the properties of the expected tuple vs the actual tuple are not in the same order

See below where only the value of a changes.

- Expected
+ Received

  (some {
-   d: u1010000000,
-   a: u1,
+   b: u1013,
    c: u75,
-   b: u1013
+   a: u0,
+   d: u1010000000
  })

With this PR, it would display

- Expected
+ Received

  (some {
-   a: u1,
+   a: u0,
    b: u1013,
    c: u75,
    d: u1010000000
  })

Clarinet already does this sorting in expectTuple, but this is actually needed for every composite types (some, ok, err, list, tuple) and with nested values. It could be done at the clarinet level but Cl.prettyPrint already handle the recursive nature of clarity values.


Checklist

vercel[bot] commented 7 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
stacksjs-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 20, 2024 6:31pm
codecov[bot] commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (922e037) 66.37% compared to head (920efdf) 66.37%.

:exclamation: Current head 920efdf differs from pull request most recent head 3d18e85. Consider uploading reports for the commit 3d18e85 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1631 +/- ## ======================================= Coverage 66.37% 66.37% ======================================= Files 119 119 Lines 8717 8718 +1 Branches 1920 1920 ======================================= + Hits 5786 5787 +1 Misses 2806 2806 Partials 125 125 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

janniks commented 7 months ago

I'm fine with sorting. For use in tests it's great. Maybe let's add a note to not use the function for keeping keys in a specific order. (I think in some cases key order can have consequences, but less likely users will use this for it)

hugocaillard commented 7 months ago

Comment addressed @janniks @zone117x Thx for the feedback!

hugocaillard commented 7 months ago

If this can be included in the next release of stacks.js, I'd like to include it in the next release of clarinet