refactor(openapi)!: convert bundling library to openapi-cli #50
Conversation
kleyow
force-pushed
the
refactor/openapi-cli
branch
from
40b7971 to
8fdd346
Compare
kleyow
changed the title
kleyow
requested review from
elnyry-sam-k,
eoln,
lewisdaly,
sridharvoruganti and
vijayg10
as code owners
There was a problem hiding this comment.
@kleyow GJ! Approved
I put a lot of comments related to some inconsistency and lack of regexp patterns & enums inherited from the original specification.
@elnyry-sam-k @lewisdaly we should decide what to do about -> should we create issue and delegate complaints of very poor API spec quality to some board?
Or maybe we should fix these problems in api-snippets - if yes then definitely in other story...
Worth mentioning that api-snippets restructuring allows spotting these imperfections much easier than reading original spec doc...
| ``` | ||
|
|
||
| Modify swagger file to reference `api-snippets`. | ||
|
|
||
| ex. | ||
| ```yaml | ||
| Money: | ||
| $ref: /path/to/node_modules/@mojaloop/api-snippets/v1.0/openapi3/schemas/Money.yaml | ||
| $ref: /path/to/node_modules/@mojaloop/api-snippets/fspiop/v1_1/openapi3/components/schemas/Money.yaml |
There was a problem hiding this comment.
@kleyow we need to think about short paths -> maybe we can create symlinks in postinstall npm script? If we are introducing braking change this little improvement could be very handy.
There was a problem hiding this comment.
Yes I will make a ticket to investigate this.
| @@ -0,0 +1,10 @@ | |||
| title: Amount | |||
| type: string | |||
| pattern: '^([0]|([1-9][0-9]{0,17}))([.][0-9]{0,3}[1-9])?$' | |||
There was a problem hiding this comment.
this pattern doesn't allow the values with zero at the end of the decimal part, for example: 100.00, 100.10, ....
There was a problem hiding this comment.
This is by design, why is it wrong? isn't that how match works? zeroes at the end after decimals add zero value and not necessary :P isn't it?
| partyIdInfo: | ||
| $ref: ./PartyIdInfo.yaml | ||
| description: 'Party Id type, id, sub ID or type, and FSP Id.' | ||
| merchantClassificationCode: |
There was a problem hiding this comment.
in other places, merchantClassifiactionCode is restricted to have only four digits...
There was a problem hiding this comment.
yes merchantClassifiactionCode is different from FSP ID, sorry I'm not understanding the issue with that
| description: Data model for the complex type PartyComplexName. | ||
| properties: | ||
| firstName: | ||
| type: string |
There was a problem hiding this comment.
what about the FirstName type defined above? - it has quite a restricted regexp pattern and here is allowed an unrestricted string
| type: string | ||
| description: Currency of the amount. | ||
| example: USD | ||
| amount: |
There was a problem hiding this comment.
what about using Amount type defined above?
| @@ -0,0 +1,6 @@ | |||
| schema: | |||
| type: string | |||
There was a problem hiding this comment.
unrestricted string, some pattern here would be great!
| description: | | ||
| The `Accept` header field indicates the version of the API the client | ||
| would like the server to use. | ||
| description: >- |
There was a problem hiding this comment.
again unrestricted string where some regexp pattern would place here nicely
| name: Date | ||
| in: header | ||
| schema: | ||
| type: string |
There was a problem hiding this comment.
Date should be guarded by regexp
| name: FSPIOP-HTTP-Method | ||
| in: header | ||
| schema: | ||
| type: string |
There was a problem hiding this comment.
why not restricted string where a pattern or enum should be applied
| @@ -3,6 +3,6 @@ in: header | |||
| schema: | |||
| type: string | |||
| required: false | |||
| description: | | |||
| description: >- | |||
There was a problem hiding this comment.
signatures are base64 encoded string so we can guard this by regexp pattern...
|
@eoln - great work reviewing the giant PR :-).. on the issues / enhancements you mentioned as comments, can we take those separately and not include them here ( for example, regex patterns on headers, etc) |
|
@elnyry-sam-k that is my idea, move improvements to another story, we are chopping big spec here only |
No description provided.