search

mtrajano / laravel-swagger

Auto generates the swagger documentation of a laravel project based on best practices and simple assumptions
167 stars 71 forks source link

Added schema definitions, responses, support to JWT auth, Swagger UI and Docker environment #24

Closed sfelix-martins closed 4 years ago

sfelix-martins commented 4 years ago

Added

Changed

Configuring

To download the package from fork just add the repository in your composer.json:

   "repositories": [
        {
            "type": "vcs",
            "url": "https://github.com/coopersystem-fsd/laravel-swagger"
        }
    ],

and change the package version to dev-master:

"require": {
        "mtrajano/laravel-swagger": "dev-master",
}

Run composer update and the code from fork should be downloaded.

Usage

Follow the instructions on README.

Closes #18 , #7

sfelix-martins commented 4 years ago

@mtrajano Firstly, thanks for this great package.

We added some cool features to try improve it. Please review it. We are available for any clarification!

We = @sfelix-martins @bc05 @alanchristiansi

mtrajano commented 4 years ago

@sfelix-martins this is awesome, thank you so much for this great pr! I will review it all as soon as I get a chance (probably this weekend). Would you guys like to be added as a maintainer for the package? I have been super busy with work lately and unfortunately have been able to devote as much time to the package as I wish I could, but it seems like you guys have been getting some use from it and understand the codebase pretty well. Let me know, thanks!

sfelix-martins commented 4 years ago

@mtrajano I really undersand you! I'm in the same situation to maintain my packages. Thanks to our organization Coopersystem for that.

Would you guys like to be added as a maintainer for the package?

Would be great. If you are comfortable with this we accept this challenge.

mtrajano commented 4 years ago

@sfelix-martins I am about halfway with the review so far in terms of number of line changes, so far really exciting stuff!! Gong to try really hard to finish this by tomorrow since I'll be a lot busier again during the weekday. So far everything looks great, just some general comments under review dealing with questions on implementation, feedback etc.. Also really happy to hear that you guys are willing to help maintain the package! Going to get through the review, merge and then add you guys to the maintainer's list. Will it just be you, @bc05 and @alanchristiansi? Thanks ahead of time!

sfelix-martins commented 4 years ago

@mtrajano Thanks for review! We'll answer ASAP and make adjustments if necessary.

bc05 commented 4 years ago

@mtrajano thanks for review.

mgamal92 commented 4 years ago

When this PR will be merged ?

mtrajano commented 4 years ago

@mgamal92 hopefully very soon 🙏

@sfelix-martins would you mind checking the box that says "Allow edits from maintainers", I had to push a bug fix to master and wanted to fix the conflict it created, and just clean some small things up before merging. Thanks!

sfelix-martins commented 4 years ago

@mgamal92 hopefully very soon

@sfelix-martins would you mind checking the box that says "Allow edits from maintainers", I had to push a bug fix to master and wanted to fix the conflict it created, and just clean some small things up before merging. Thanks!

@mtrajano I can't see this option :disappointed:

@bc05 can you see it?

mtrajano commented 4 years ago

@sfelix-martins it should be on the sidebar (https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/allowing-changes-to-a-pull-request-branch-created-from-a-fork), I believe only you should be able to change it since you created the pr. Alternatively you can add me as a contributor to the fork and I'd be able to push as well, thanks.

sfelix-martins commented 4 years ago

@sfelix-martins it should be on the sidebar (https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/allowing-changes-to-a-pull-request-branch-created-from-a-fork), I believe only you should be able to change it since you created the pr. Alternatively you can add me as a contributor to the fork and I'd be able to push as well, thanks.

I followed this tutorial, but this option is hidden for me. I don't know if this behavior is because this fork is from organization... Well I'll try add you as a contributor.

sfelix-martins commented 4 years ago

@mtrajano I invited you to contribute with the fork.

sfelix-martins commented 4 years ago

@mtrajano Thanks for good changes.

I saw that occurred and error on travis build. Here has an example that can help you.

mtrajano commented 4 years ago

@sfelix-martins oh nice yes that is a much nicer solution, I'll update ci

mgamal92 commented 4 years ago

Can I help in changes to make the process fast ? because I need this PR to be merged to can use it

sfelix-martins commented 4 years ago

@mtrajano Please, resolve the review conversations that you already handled, to be more easy to see the really pending reviews.

mtrajano commented 4 years ago

@mgamal92 @sfelix-martins hey sorry will resolve the issues I tackled already. The 2 things missing that are the most pressing are: 1. changing the example data logic from create to make and add tests for those (can also kill the transaction logic). 2. for the logic of getting the model relationships we should definitely not be calling random model methods in order to not cause any side effects (can potentially increment things on the db, add/remove data, etc..). Potential solutions would be to either rely on the return type hints to be of instance HasOne etc.. (prefer this one) or rely on a new annotations, and again just adding tests for these.

Some other ones I was hoping to get to before merging are: 1. making ignoredRoutes also take regex since this was a feature before and we're removing it in this pr, 2. Making the response logic more flexible (being able to config these etc..), am currently working on this one and adding tests. Finally some general clean up and fixing the README documentation.

Sorry again for the time it's taking to merge this one, I was actually away at a conference last week so I only had a few hours here and there to get stuff out. I have a lot more time until Wednesday so I was hoping to merge it by then. But yes if you guys can tackle some of these I'd really appreciate it and it'd speed the process a lot. Thanks!

mgamal92 commented 4 years ago

Please clarify what problem about response ?

mtrajano commented 4 years ago

@mgamal92 nothing per se, it would just be nice if the consumer could configure which route methods map to which response codes and what their descriptions are. But again, that could come at a future release. I already fixed the example logic and I'm working on the model relationships logic so the only thing left to do would be to fix the docs to match the new config structure and adding the ability to filter by regex (not just route name). This should hopefully be done by tomorrow 🤞

mgamal92 commented 4 years ago

So I understood from that, the PR will be finished merged tomorrow, right?

On Tue, Feb 11, 2020, 4:05 AM Mauricio Trajano notifications@github.com wrote:

@mgamal92 https://github.com/mgamal92 nothing per se, it would just be nice if the consumer could configure which route methods map to which response codes and what their descriptions are. But again, that could come at a future release. I already fixed the example logic and I'm working on the model relationships logic so the only thing left to do would be to fix the docs to match the new config structure and adding the ability to filter by regex (not just route name). This should hopefully be done by tomorrow 🤞

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mtrajano/laravel-swagger/pull/24?email_source=notifications&email_token=AHWBLWAEQSNSF2UAGICSLK3RCIBYJA5CNFSM4KNJELW2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELK7WGY#issuecomment-584448795, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHWBLWDNECFJ2KDREKMJGD3RCIBYJANCNFSM4KNJELWQ .

mtrajano commented 4 years ago

Pushed some more changes out, only thing missing is just to fix the README and add a config called parseModels to disable invoking the model methods by default. Thanks for bearing with me all

mtrajano commented 4 years ago

@sfelix-martins Sorry for the late response to this. I fixed the docs to match the match the new config layout. However there are still a couple of flaky tests ever since you changed the assertion to check that the example property is empty vs null. It always passes locally but seems to fail every now and then in CI, I tried sshing into a 7.2/7.3 php docker container and tried running the test there but it was still passing pretty consistently. So I chose to skip those tests for now.

I don't want to keep this pr open much longer and keep adding changes to this since I think it's getting a little out of control so I opened a v0.7 branch and I think we can merge to that in the meantime. I'll cut a dev release for 0.7 that way people who want to use/test the new features are able to do so and we're able to clean up, fix the tests, etc.. and when it's stable we'll merge it into master, does that sound ok? I'll also add you guys as collaborators as well, thank you so much for this awesome pr and sticking around!

sfelix-martins commented 4 years ago

@mtrajano ok man. It looks good. Thank you for develop this very good package. I think we can finally merge.