-
Notifications
You must be signed in to change notification settings - Fork 148
Conversation
Hello, Thank you for your contribution. It would be great if you could add some tests. Also, the documentation needs to be modified to reflect the changes and the new available params. Finally, it looks like you basically took the code from api_param and copy/pasted it to create the two new types. Isn't it possible to reduce this duplication in one way or another? |
@NicolasCARPi where is the documentation? I was not able to find it in the project, though I may have been looking in the wrong place for it. |
Here you go: https://github.com/apidoc/apidocjs.com |
@NicolasCARPi I am working on updating the documentation. When I make a PR, do you want me to reference this PR (with a URL or something to that extent)? |
@NicolasCARPi I have finished making the changes you requested, with the exception of the documentation on the other project. |
@SWheeler17 This is already much better. I'll merge this after you propose a documentation change. It's important to keep the documentation in sync with the code, and because you are adding functionality, it is also your responsability to modify the documentation to indicate the usage of such params and in which case they are useful. |
Ok, @NicolasCARPi how do you want me to link this PR to the PR on the documentation PR? I was going to add the link of this PR to that (eventual) PR unless you want me to do something else. |
Whatever works for you! |
Please ping me if this has not been reviewed in a week. |
@NicolasCARPi this has not been merged and does not appear to have been reviewed either. |
Here is a good example https://github.com/apidoc/apidoc-core/blob/master/lib/workers/api_success_title.js |
@tommy87 I updated the two files you commented on, addressing your comments. Please review it again. |
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.
Looks much better, but i still need to run tests.
@rottmann @NicolasCARPi
is there any chance to enable CI / Actions in this project which run the tests automatically?
@tommy87 Yes sure! Github actions or CircleCI. |
@NicolasCARPi ah i thought it must be enabled first, but when it is similar to gitlab i could create a simple test in the next days |
I would favor github actions as they do not require a third party account/access. There used to be Travis CI IIRC on this repo. But maybe @rottmann pulled the plug. |
@SWheeler17 i have create some CI actions on the dev branch. Could you rebase your branch so that the tests are triggered @NicolasCARPi shouldnt the dokumentation be updated before pulling? |
@tommy87 I have rebased and the tests seem to have run successfully. The documentation PR is here: apidoc/apidocjs.com#34 (for future reference). |
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.
Just correct the name and then everythign should be fine
Oops, fixed @tommy87, my bad. |
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.
@NicolasCARPi i think it is ready for pulling
What is the likelihood of this PR being pulled within the next few days? |
100% :p |
This is for issue #24 which suggested an apiBody and apiQuery be created instead of using (the general tag) apiParam.