-
Notifications
You must be signed in to change notification settings - Fork 2
chore: generate API level classes from KERIA OpenAPI #59
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
Conversation
iFergal
left a comment
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.
@Sotatek-DucPhung Feel free to reply to my comments, but latest priorities as shared on Slack take priority tomorrow. Patrick can maybe pick this up later.
|
Not sure why one of the e2e tests is failing |
iFergal
left a comment
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 have not individually reviewed the generated classes as there are too many.
Our goal is to start leveraging these types to replace Object throughout the repository - this may highlight issues in itself - but I will also start reviewing the used types batch by batch in each PR. Subsequent PRs will be small to be reviewable.
The risk should be somewhat low anyway as these types are already in use by Signify-TS (or starting to be in use).
For further security reasons, I can do a full regeneration of the types later when all subsequent PRs have landed.
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.
Checked files used from PR #64
- It seems a default (Java here and TS here) causes @nullable - do you know why that would be?
- I noticed transferable and windexes have JsonNullable - Signify-TS also has | null in the type union…
- Can you check with @Sotatek-Patrick-Vu here on why those might be nullable like that - I don’t think they should be?
Checked:
----------
- IdentifiersPostRequest
- IdentifiersNamePutRequest
- EndrolesAidPostRequest
- StateEERecord
- Identifier
- Tier
- KeyStateRecord
- RandyKeyState
- SaltyState
- GroupKeyState
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.
@iFergal for transferable and windexes they can be optional but not nullable. I think we need to change the type in Keria for them then Java and TS types will be auto updated
you can see here: https://github.com/WebOfTrust/keria/blob/54611c911b8bf5b2606e170fb85c2f5854b089eb/src/keria/app/aiding.py#L1252
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.
@Sotatek-Patrick-Vu That's true - but it's always called with full=True so they should always be provided, right?
No description provided.