Skip to content

feat: sort tuple properties in cl pretty print#1631

Merged
janniks merged 2 commits intomainfrom
feat/sort-properties-in-cl-pretty-print
Feb 23, 2024
Merged

feat: sort tuple properties in cl pretty print#1631
janniks merged 2 commits intomainfrom
feat/sort-properties-in-cl-pretty-print

Conversation

@hugoclrd
Copy link
Contributor

@hugoclrd hugoclrd commented Feb 19, 2024

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

  • Unit tested updated code paths
  • Tagged 1 of @janniks for review

@hugoclrd hugoclrd requested a review from janniks February 19, 2024 18:34
@vercel
Copy link

vercel bot commented Feb 19, 2024

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
Copy link

codecov bot commented Feb 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

❗ 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
@@           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           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hugoclrd hugoclrd requested a review from zone117x February 20, 2024 14:34
@janniks
Copy link
Contributor

janniks commented Feb 20, 2024

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)

@hugoclrd
Copy link
Contributor Author

Comment addressed @janniks @zone117x
Thx for the feedback!

@hugoclrd
Copy link
Contributor Author

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

Copy link
Contributor

@zone117x zone117x left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

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