-
Notifications
You must be signed in to change notification settings - Fork 516
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
Improve api documentation and error handling #2690
Conversation
Signed-off-by: jamshale <[email protected]>
Signed-off-by: jamshale <[email protected]>
Signed-off-by: jamshale <[email protected]>
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.
The snake_case vs camelCase changes have me a little nervous since I vaguely recall discussions about the names but I've been away from this long enough that I trust your judgement over my own on the details here. One small comment. Otherwise, LGTM!
@@ -116,13 +131,17 @@ class Meta: | |||
issuer_id = fields.Str( | |||
description="Issuer Identifier of the credential definition or schema", | |||
data_key="issuerId", | |||
example=INDY_OR_KEY_DID_EXAMPLE, |
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'm a little hesitant about the only examples showing as old Indy values. Is it possible to provide more than one example so we could do something like did:example:123
?
Also, I believe most of these fields should be in the metadata
kwarg now after updates to marshmallow, as a side note. That change probably belongs in a separate PR.
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 had thought about that. I was thinking when we add more methods we could figure something out for the other examples. For the metadata stuff I saw both methods used and wasn't sure the correct way.
I think the casing caused some confusion when this was getting implemented. The camel casing still applies for the anoncred objects so I'm pretty sure it's good. I basically just removed any discrepancies with search params and outer objects being mixed cases anymore. |
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.
Looks good!!! Regarding @dbluhm 's comments (snake vs camel case, number of examples, etc) if there is anything we need to follow up later we should add as new issues.
Quality Gate passedKudos, no new issues were introduced! 0 New issues |
Add examples and descriptions to the swagger docs. Mostly used the indy examples.
There was some confusion with request params object attributes with camel vs snake casing. The anoncreds library does camel case like the spec decribes. Everywhere else has snake case. I changed everything so that only the inner anoncreds schema, cred_def and rev_reg_def objects are in camel case. I think this makes things a bit more clear but it is still a bit confusing when you are used to the indy api.
I changed the POST rev_reg_def endpoint to have a wrapper like schema and cred_def with an options object where you can put the endorser connection id.
Changed the GET all creds defs return object to be in a wrapper like schemas.
Fixed a couple mistakes and improved the error handling. I think a majority of errors give information about the cause now. Can still get 500 errors with unexpected param values.