Skip to content
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: Adds validation_state to the json reports from the Reader #930

Merged
merged 6 commits into from
Feb 19, 2025

Conversation

gpeacock
Copy link
Collaborator

@gpeacock gpeacock commented Feb 18, 2025

This is also a cleanup of the new API, removing ManifestStore, ManifestStoreReport, and Store from the public API unless the v1_api feature is used.
These are steps toward a version 1.0 SDK.

chore: remove Store and ManifestStore from the non-v1 public SDK
refactoring and code removal
Copy link

codecov bot commented Feb 18, 2025

Codecov Report

Attention: Patch coverage is 74.90909% with 69 lines in your changes missing coverage. Please review.

Project coverage is 78.74%. Comparing base (bd737c8) to head (c1303e0).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
sdk/src/reader.rs 72.61% 43 Missing ⚠️
sdk/src/manifest_store_report.rs 10.00% 9 Missing ⚠️
make_test_images/src/compare_manifests.rs 0.00% 7 Missing ⚠️
sdk/src/store.rs 86.20% 4 Missing ⚠️
sdk/src/validation_results.rs 94.44% 3 Missing ⚠️
sdk/src/validation_status.rs 70.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #930      +/-   ##
==========================================
- Coverage   78.94%   78.74%   -0.21%     
==========================================
  Files         147      147              
  Lines       34842    34965     +123     
==========================================
+ Hits        27507    27534      +27     
- Misses       7335     7431      +96     

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

Copy link
Contributor

@tmathern tmathern left a comment

Choose a reason for hiding this comment

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

Out of an abundacne of caution, I would suggest to verify this does not break c2pa-c and c2pa-python before emrging it (breaking the builds and/or the tests).

And even if it breaks something, then we'd know what to fix too.

@gpeacock
Copy link
Collaborator Author

I build and ran tests using this branch with c2pa-c and c2pa-python, everything built without errors and all tests passed.

@gpeacock gpeacock merged commit 91f24a9 into main Feb 19, 2025
29 of 31 checks passed
@gpeacock gpeacock deleted the gpeacock/remove_manifest_store branch February 19, 2025 19:34
@scouten-adobe scouten-adobe mentioned this pull request Feb 19, 2025
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