OpenAPI support #122
OpenAPI support #122
Conversation
|
How can we test this PR on an existing project? |
|
For testing the functionality of the branch in an existing project, I think that approach could work https://docs.npmjs.com/files/package.json#git-urls-as-dependencies |
|
@kalinchernev thank you, it looks working on my project |
| const v3 = [...v2, 'components']; | ||
|
|
||
| if (specification.openapi) { | ||
| specification.openapi = specification.openapi; |
There was a problem hiding this comment.
What is the goal of this line ? Same question for the line 29 (specification.swagger = specification.swagger;) ?
There was a problem hiding this comment.
@alexisab thanks a lot for making reviews!
In overall, I want to refactor much of the codebase to avoid reassigning values and mutating objects, but at this point I decided to keep old ways of organizing properties to avoid going too far changing things which are not related to the open api specification support.
The idea behind these lines is that that, if the user has provided any of these 2 properties, swagger or openapi in the definition options, respect them. Otherwise the rest of the logic adds and removes properties ...
| function filterJsDocComments(jsDocComments) { | ||
| const swaggerJsDocComments = []; | ||
|
|
||
| for (let i = 0; i < jsDocComments.length; i += 1) { |
There was a problem hiding this comment.
IMHO You should use for ... of loop, it's more readdable
There was a problem hiding this comment.
I agree, there are many other places where the codebase should be improved! Again, didn't refactor too much in order to focus on the issue at hand...
| const filterJsDocComments = require('./filterJsDocComments'); | ||
| const convertGlobPaths = require('./convertGlobPaths'); | ||
|
|
||
| function isEmptyObject(obj) { |
There was a problem hiding this comment.
What do you think about this : const isEmptyObject = obj => Object.keys(obj).length ?
There was a problem hiding this comment.
Not quite, the idea is to find properties with empty objects, not whether properties exist.
if your suggestion is isEmptyObject2:
const obj = {foo: {}}
undefined
isEmptyObject(obj)
true
isEmptyObject2(obj)
1
1 is truthy, which is false for what's the check intention.
| * @param {object} improvedSpec - The cleaned version of the inputSpec | ||
| */ | ||
| function cleanUselessProperties(specification) { | ||
| const spec = JSON.parse(JSON.stringify(specification)); |
There was a problem hiding this comment.
Why stringify then parse ?
Params in the jsdoc comment are not correct.
There was a problem hiding this comment.
This is a typical pattern in JS for cloning entities recursively.
There was a problem hiding this comment.
@kibertoad is right. Deep clone without reaching for a third party library. That's one of the very few places where I reached for changing the previous logic of mutating objects. There are many other places where this change will make the library better, but I didn't want to go deep in the rabbit hole changing too much unrelated code.
Please enter the commit message for your changes. Lines starting
|
As so far I've seen feedback about that it's working and there are tests to double-check this, I'm merging and releasing this one. |
Reviews are welcome.
The pull request takes ideas from discussions in #65, however support both v2 and v3 of the specification is targeted with full backwards compatibility. Code is re-organized a bit to hopefully make the project more maintainable through change of specification versions.
To facilitate unit testing jest-snapshot is used. It gives better feedback when failing tests because of properties changes with comparison to current tests of string comparisons.
Examples for tests of the OpenAPI specification are taken from the official repository of the specification.