feat: Pass settings in the reader and builder constructor #40
Conversation
Remove global settings
Add TODO for methods which do not yet support settings in the constructor
| } | ||
|
|
||
| static withJson(json: Manifest): Builder { | ||
| static withJson(json: Manifest, settings?: C2paSettings): Builder { |
There was a problem hiding this comment.
I know the context APIs are coming.
I wonder if you'll end up then changing the second argument to a generic "options" object that holds both settings and context, or only the context. Hard to guess since the API is not totally settled.
But I wonder if you would already be better off having a placeholder options here on which you could put anything so signatures don't change.
Not a change request, just a point to ponder.
There was a problem hiding this comment.
I don't even know if there is a meaningful distinction between context and settings at this point.
js-src/Settings.ts
Outdated
| /** | ||
| * Load settings from a JSON string and apply them globally. | ||
| * @param json The JSON string containing the settings configuration | ||
| * Create a settings object with trust configuration. |
There was a problem hiding this comment.
| * Create a settings object with trust configuration. | |
| * Create a Settings object with trust configuration. |
README.md
Outdated
| // Create with custom settings | ||
| const settings = { | ||
| verify: { | ||
| verify_after_sign: false |
There was a problem hiding this comment.
We should probably explain why we set this to false... I don't know for how long we'll keep this value to false either. (Target is to have that one default to true).
There was a problem hiding this comment.
I just added a link to the rust SDK for how the settings work. No point in describing something that isn't nailed down in multiple places.
|
LGTM |
Remove global settings as this SDK is not yet stable.