search

theoomoregbee / sails-hook-swagger-generator

A tool to help generate Swagger specification documentation based on OAS 3.0 for Sails APIs
MIT License
78 stars 33 forks source link

fix(core): refactor to work with Sails 1.0, OpenAPI 3, JSDoc (work in progress) #44

Closed danielsharvey closed 4 years ago

danielsharvey commented 5 years ago

Refactor hook initialisation to run after Sails ready in order to ensure that sails.getActions() returns appropriate content.

Replace removed sails.controllers with equivalent data structure derived from sails.getActions() (work in progress).

Utilise model attribute columnType to infer more type info (work in progress).

Addresses https://github.com/theo4u/sails-hook-swagger-generator/issues/11#issuecomment-437683002 and https://github.com/theo4u/sails-hook-swagger-generator/issues/39

closes #11 closes #39 closes #51

danielsharvey commented 5 years ago

This needs further checking to ensure it is complete and covers all aspects of mapping actions and model attributes, but I've created a PR in case of interest and for comment on direction.

theoomoregbee commented 5 years ago

@danielsharvey thanks.

Replace removed sails.controllers with equivalent data structure derived from sails.getActions() (work in progress).

See my thoughts about it here: https://github.com/theo4u/sails-hook-swagger-generator/issues/18#issuecomment-370853338

theoomoregbee commented 5 years ago

With this, there's no support for sails v0.12.x and maybe we want to do some fixes or feats that cuts across both version, I don't think it will be possible again with

-    "sails": "~0.11.0",
+    "sails": "^1.1.0"

Wdyt?

danielsharvey commented 5 years ago

Apologies for long delay responding. I suspect maintaining support for both 0.12.x and 1.x will be difficult in the same release. Maintaining 2.x for Sails v0.12.x and releasing 3.x for 1.x support might be pragmatic.

With this, there's no support for sails v0.12.x and maybe we want to do some fixes or feats that cuts across both version, I don't think it will be possible again with

-    "sails": "~0.11.0",
+    "sails": "^1.1.0"

Wdyt?

Will push updated support shortly.

danielsharvey commented 5 years ago

@theo4u, by way of update: This is progressing although not as quickly as I would like.

Question: Do you have an opinion on moving from Swagger to to OpenAPI 3?

theoomoregbee commented 5 years ago

Question: Do you have an opinion on moving from Swagger to OpenAPI 3?

No, I've not given this a thought

danielsharvey commented 5 years ago

@theo4u Quick update as this has taken longer than anticipated. I found issues with using sails.getActions() and have moved to listening to router:bind events to capture routes for blueprint actions. I've also worked to upgrade output to OpenAPI 3.

theoomoregbee commented 5 years ago

@danielsharvey cool 👍 , can you push so I can try it out

ghost commented 5 years ago

There were the following issues with this Pull Request

You may need to change the commit messages to comply with the repository contributing guidelines.

🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

ghost commented 5 years ago

There were the following issues with this Pull Request

You may need to change the commit messages to comply with the repository contributing guidelines.

🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

ghost commented 5 years ago

There were the following issues with this Pull Request

You may need to change the commit messages to comply with the repository contributing guidelines.

🤖 This comment was generated by commitlint[bot]. Please report issues here.

Happy coding!

danielsharvey commented 5 years ago

@theo4u, this turned out to be a more significant refactor that I expected once I got started now that I have included Sails 1.0, OpenAPI 3 and JSDoc.

Updates to the tests are a work-in-progress - I have not updated the all tests, only the bootstrap test so far.

To run bootstrap only, use:

node ./node_modules/mocha/bin/mocha test/bootstrap.test.js

I have tested quite extensively on a project I'm using this for, but the test output produced in swagger/swagger.json by the bootstrap test is OK, and has been tested against:

eduardotamaki commented 5 years ago

Hi @danielsharvey, just tried your code and despite some warnings, it worked to generate the open api from blueprints of my sails 1.0 application.

Keep up with the good work!

danielsharvey commented 5 years ago

Thanks for the feedback @eduardotamaki. Feel free to let me know any specific warnings - I've dealt with a reasonable number so far but happy to address (as noted, I know that work is needed to finished updating the tests).

theoomoregbee commented 5 years ago

@theo4u, this turned out to be a more significant refactor that I expected once I got started now that I have included Sails 1.0, OpenAPI 3 and JSDoc.

Updates to the tests are a work-in-progress - I have not updated the all tests, only the bootstrap test so far.

To run bootstrap only, use:

node ./node_modules/mocha/bin/mocha test/bootstrap.test.js

I have tested quite extensively on a project I'm using this for, but the test output produced in swagger/swagger.json by the bootstrap test is OK, and has been tested against:

Will try it out today and wait for the tests(if i've time over the weekend will assist too) so we know how to release this, in case of any breaking changes

EvanTimmermann commented 4 years ago

Hey @theo4u and/or @danielsharvey, Has this PR been looked at recently? I just stumbled across it and I'm interested in seeing this implemented.

danielsharvey commented 4 years ago

Hey @theo4u and/or @danielsharvey, Has this PR been looked at recently? I just stumbled across it and I'm interested in seeing this implemented.

@EvanTimmermann I regularly use this internally but I've not worked on the tests unfortunately. I will have some space in several weeks to look at it.

enricodeleo commented 4 years ago

I was having problems with the original branch with sails 1.2.3. None of the blueprint rest endpoints was documented.

With this fork, everything works like a charm so THANKS!

I can't wait to see this merged!

theoomoregbee commented 4 years ago

Will try looking at this coming weekend so we can merge it in

danielsharvey commented 4 years ago

Thanks for the feedback @enricodeleo. As I mentioned previously, I've not had time to work on the automated tests. But I will make sure I contribute quickly if other issues found (and I can help!) - I've been using myself for quite some time.

theoomoregbee commented 4 years ago

:tada: This issue has been resolved in version 3.0.0 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket: