Remove dependency on macros package from c api #352
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I'm extending the tests in didkit-c to validate the values returned are still what we expect to be returned. The changes to remove the didkit macros were more complex than expected. |
CharlesShuller
requested review from
theosirian,
sbihel,
cobward and
todd-spruceid
sbihel
requested changes
May 9, 2023
There was a problem hiding this comment.
I think the PR does a bit too much compared to what it was supposed to do, even though it's mostly for the better. Just makes the review a bit harder.
Two additional points:
- the actual removal of the macro is missing; and
- Clippy raises a few warnings, the base branch isn't up to date with main so that's why the CI isn't failing. Can you address the warnings raised from this PR please?
todd-spruceid
approved these changes
May 9, 2023
theosirian
approved these changes
May 9, 2023
PR Feedback indicated that the long 'and_then' chains used to marshal C strings into rust data structures were difficult to read. I've replaced them in two functions to see if reviewers prefer the new function calls. Impacted functions are: * didkit_key_to_did * didkit_vc_issue_credential
PR Feedback indicated that the long 'and_then' chains used to marshal C strings into rust data structures were difficult to read. I've replaced them in two functions to see if reviewers prefer the new function calls. Impacted functions are: * didkit_key_to_did * didkit_vc_issue_credential
…ceid/didkit into feat/remove-didkit-macro-from-c-api
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Removes dependency on didkit_macros from the C API.
Tests covering this are in the didkit-c repo, PR: https://github.com/spruceid/didkit-c/pull/3