Add a cwt.utils.ResolvedHeaders to allow tstr header labels and values#636
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #636 +/- ##
=======================================
Coverage 97.34% 97.34%
=======================================
Files 32 32
Lines 3352 3357 +5
=======================================
+ Hits 3263 3268 +5
Misses 89 89 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
|
@dajiaji I believe the change is now ready for review, thank you for your patience! |
dajiaji
left a comment
There was a problem hiding this comment.
@achamayou Sorry for the delayed review.
I have no concerns about the introduction of ResolvedHeader. It looks like a reasonable solution.
All existing tests are passing, so I will go ahead and merge it.
Thank you for your contribution, including the addition to the README.
|
@dajiaji thank you for taking the time to review and merge the PR, this is a much appreciated solution for users like me who need to make use of strings in headers in some cases. |
Option B from #634, backwards compatible with existing code.
The code so far only expects integer labels, and always encodes string values to bstr when present. String labels are only allowed as shortcuts to integer labels.
With this change, the current behaviour does not change, but users can wrap their header dict in a ResolvedHeader object send it directly to CBOR encoding, without short string lookup or string value encoding.
It seems to me that in the long run, it would be preferable to switch and adopt this behaviour by default as proposed in #634, but this is a more disruptive change for users of course, and there are perhaps not that many who use string labels compared to short string names.