-
Notifications
You must be signed in to change notification settings - Fork 23
Expose add example read.py script #207
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
base: main
Are you sure you want to change the base?
Conversation
…ng custom settings (like a certificate trust list).
…est and verify it.
src/c2pa/__init__.py
Outdated
| 'read_ingredient_file' | ||
| 'read_ingredient_file', | ||
| 'load_settings', | ||
| 'version' |
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.
Sidenote: Could you move it next to sdk_version?
sdk_version is the version of the underlying native library.
I am also curious why you need this one exported?
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.
For logging purposes. Like to use like this: logger.info("Initializing C2PA reader using {c2pa.version()}"). I can use sdk_version() but I liked that version() gives more details (c2pa-c-ffi/0.73.1 c2pa-rs/0.73.1 vs 0.73.1). Let me know
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.
I see! This is mostly for historical reasons that we have 2.
c2pa-c-ffi and c2pa-rs are in lockstep release (we now always release c2pa-c-ffi when we release c2pa-rs, because they share a build system to create the native libraries), so sdk_version() give you their shared version number (so, 0.73.1 for instance). If I clarify that in the docs and comments, do you still wish to have both version and sdk_version then?
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.
I thought it was simply forgotten, but it's really not a dealbreaker to not have it
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.
I prefer we'd not add it then. I will clarify the docs (hopefully this week) regarding the version and how it works then.
Once the "new" version is removed, this PR would be good to go I think (I'll have the tests run on the change before that).
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.
Like this: #211
(And then we'd merge this PR to keep your nice examples!).
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.
Alright I removed the changes to __init__.py, should be good now
|
fyi, there is also an example repo: https://github.com/contentauth/c2pa-python-example. |
This is good to know and in my opinion should be advertized in this repo somehow (like in the README). I would not have found it if you didn't tell me! |
load_settings(), version() and add example read.py script
Add an example read.py script to show how to read a file's C2PA manifest and verify it.