-
Notifications
You must be signed in to change notification settings - Fork 58
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
feat: Move time stamp code into c2pa-crypto #696
Conversation
Blocked by need for TS verification
# Conflicts: # internal/crypto/src/lib.rs
…s-changes-through
# Conflicts: # internal/crypto/src/lib.rs
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #696 +/- ##
==========================================
- Coverage 81.52% 81.39% -0.13%
==========================================
Files 104 109 +5
Lines 30634 30711 +77
==========================================
+ Hits 24973 24996 +23
- Misses 5661 5715 +54 ☔ View full report in Codecov by Sentry. |
@mauricefisher64 @mikeklein13 do we have any sample time stamp data that we could use to achieve independent code coverage for this? |
…s-changes-through
Per @mauricefisher64, look at the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your hard work on this! I have a couple of suggestions that I think could help improve the library’s structure and usability:
Decouple HTTP I/O:
Consider focusing the library on just building HTTP requests, parsing responses, and verifying them. This would simplify the library’s structure and make the code easier to test. For users needing more convenience, you might offer a way to pass in a function that takes an HTTP request and returns an HTTP response. This approach would let users choose any HTTP client they prefer while shifting connection handling to the user rather than the library. (Won't implement in this PR. Tracking as CAI-7305.)
Leverage standardized types:
It might be worth using the standardized types from the http crate for request building and parsing responses. This could enhance compatibility and consistency while making the library easier to integrate into other projects. (Won't implement in this PR. Tracking as CAI-7306.)
Decouple verifying logic from response parsing:
Combining both responsablities leads to complicated code that's difficult to fully test. (Won't implement in this PR. Tracking as CAI-7307.)
aa0fdc2
to
d0276d1
Compare
@mauricefisher64 Almost there: Still need to bring over |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed offline. Suggested updates will be addressed after this is merged.
40a281b
to
b7895e4
Compare
No description provided.