Skip to content

Conversation

fboucquez
Copy link
Contributor

  • Removed open api generated code
  • Removed open api templates
  • Added open api generated dependency
  • Updated open api imports
  • Patched a few client call lines
  • Added TransactionHttp tests mocking open API route clients.

Generator:
https://github.com/NEMStudios/nem2-open-api-generator

NPM Package:
https://www.npmjs.com/package/nem2-sdk-openapi-typescript-node-client

Maintainer @rg911 @fboucquez

Removed open api generated code
Removed open api templates
Updated open api imports
Patched some client call lines
Added TransactionHttp tests mocking open api route clients.
@fboucquez fboucquez requested review from evias and rg911 December 30, 2019 18:34
Copy link
Contributor

@rg911 rg911 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added small change to fix #405. sorry @fboucquez I missed that issue in our previous discussion.
Otherwise this PR looks good to me.

@rg911
Copy link
Contributor

rg911 commented Dec 31, 2019

@evias we have been discussing this in previous closed PRs. I second the idea wrapping OpenAPI codes into a lib package. A few benefit here:

  1. Future http client changes made easier, and we can avoid big changes in PRs
  2. Standard OpenAPI generator template has got a lot lint issues,
  3. Improve the test coverage up to more than 20%
  4. Controlled lib version
  5. First setup for automated CI / CD

@evias
Copy link
Contributor

evias commented Dec 31, 2019

I suggest to have the package reviewed for adding to nemtech organization such that its content follows the same review as the SDK/catbuffer generators.

@rg911
Copy link
Contributor

rg911 commented Dec 31, 2019

I suggest to have the package reviewed for adding to nemtech organization such that its content follows the same review as the SDK/catbuffer generators.

Certainly can. Buy the way, the package is generated using official openAPI generator and standard template. Content would be the same as before.

* The transaction status group "failed", "unconfirmed", "confirmed", etc...
*/
public readonly group: string,
public readonly group: TransactionStateTypeEnum,
Copy link
Contributor Author

@fboucquez fboucquez Dec 31, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TransactionStatus is part of the SDK API. If we add the enums here we are exposing generated code to the clients when everything other open API code is wrapped/hidden. This would make it harder to update if we want to change the generated clients eventually. For example, Java has 2 different generated clients, another possible client could be direct to DB one. Not a big issue at this stage.

@rg911 rg911 merged commit 1c200b3 into symbol:master Jan 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants