Skip to content

Comments

Add a cwt.utils.ResolvedHeaders to allow tstr header labels and values#636

Merged
dajiaji merged 11 commits intodajiaji:mainfrom
achamayou:let_already_resolved_header_through
Sep 7, 2025
Merged

Add a cwt.utils.ResolvedHeaders to allow tstr header labels and values#636
dajiaji merged 11 commits intodajiaji:mainfrom
achamayou:let_already_resolved_header_through

Conversation

@achamayou
Copy link
Contributor

@achamayou achamayou commented Aug 27, 2025

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.

@codecov
Copy link

codecov bot commented Aug 27, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.34%. Comparing base (40fe8e0) to head (1ac845e).
⚠️ Report is 14 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@achamayou achamayou changed the title Sketch out ResolvedHeaders Add a cwt.utils.ResolvedHeaders to allow tstr header labels and values Aug 29, 2025
@achamayou achamayou marked this pull request as ready for review August 29, 2025 15:16
@achamayou
Copy link
Contributor Author

@dajiaji I believe the change is now ready for review, thank you for your patience!

Copy link
Owner

@dajiaji dajiaji left a comment

Choose a reason for hiding this comment

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

@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 dajiaji merged commit 601fd48 into dajiaji:main Sep 7, 2025
19 checks passed
@achamayou
Copy link
Contributor Author

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

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