search

alysivji / falcon-apispec

apispec plugin that generates OpenAPI specification (aka Swagger Docs) for Falcon web applications.
MIT License
44 stars 31 forks source link

Added ability to use suffixes with a controller that has been already registered without suffixes. #23

Open ebensonwwg opened 4 years ago

codecov-commenter commented 4 years ago

Codecov Report

Merging #23 into master will not change coverage. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master       #23   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            3         3           
  Lines           40        53   +13     
=========================================
+ Hits            40        53   +13
Impacted Files Coverage Δ
falcon_apispec/falcon_plugin.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update a5e665e...346f65b. Read the comment docs.

alysivji commented 4 years ago

Thanks for making this PR!

It's July 4th long weekend, won't be able to review until next weekend. Appreciate your understanding.

ebensonwwg commented 4 years ago

Hope you enjoyed the long weekend. I appreciate your update and look forward to your comments and merge this weekend.

ebensonwwg commented 4 years ago

I will follow up on Monday with my coworker.

Eric Benson

From: Aly Sivji notifications@github.com Sent: Saturday, July 11, 2020 6:35:46 PM To: alysivji/falcon-apispec falcon-apispec@noreply.github.com Cc: Benson, Eric Eric.Benson@grainger.com; Author author@noreply.github.com Subject: Re: [alysivji/falcon-apispec] Added ability to use suffixes with a controller that has been already registered without suffixes. (#23)

@alysivji requested changes on this pull request.

In README.mdhttps://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Falysivji%2Ffalcon-apispec%2Fpull%2F23%23discussion_r453245764&data=02%7C01%7Ceric.benson%40grainger.com%7Ce336f8a04ae542737b3408d825f32349%7C48d1dcb6bccc4365ac7fb937a7f7fd71%7C0%7C0%7C637301073506730807&sdata=QKnkRL6lrX0IuyufHaU8ipVqc2HSdLZlBf%2FY2o0oJvY%3D&reserved=0:

+# should be passed twice so the suffix is registered +# path should be called n + 1 times where n is the number of suffixes

Unless people read this README, they are not going to know about this convention.

Is there a better way to register resources? Would prefer to do it once versus n+1 times. Also, is there is a reason it's n+1?

In README.mdhttps://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Falysivji%2Ffalcon-apispec%2Fpull%2F23%23discussion_r453245856&data=02%7C01%7Ceric.benson%40grainger.com%7Ce336f8a04ae542737b3408d825f32349%7C48d1dcb6bccc4365ac7fb937a7f7fd71%7C0%7C0%7C637301073506730807&sdata=GkLujQ4bzLPb31NulAkYEo%2BsnkuFjc8s45CnSFJAtcU%3D&reserved=0:

+spec.path(resource=random_pet_resource) spec.path(resource=random_pet_resource)

resource=pet_resource since it got renamed

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Falysivji%2Ffalcon-apispec%2Fpull%2F23%23pullrequestreview-446835157&data=02%7C01%7Ceric.benson%40grainger.com%7Ce336f8a04ae542737b3408d825f32349%7C48d1dcb6bccc4365ac7fb937a7f7fd71%7C0%7C0%7C637301073506730807&sdata=CyynEYDMdoPG4ybih8tv6unQUjIs9JiYeaHXl5FG1ew%3D&reserved=0, or unsubscribehttps://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAJ5IWGB7VLHLBBKDF25ES73R3DZNFANCNFSM4OJTMJ7A&data=02%7C01%7Ceric.benson%40grainger.com%7Ce336f8a04ae542737b3408d825f32349%7C48d1dcb6bccc4365ac7fb937a7f7fd71%7C0%7C0%7C637301073506740803&sdata=uucfY8SfwoJibZN%2Flb3RNf1XTYmvn2mOw%2FgR6Vyrc2c%3D&reserved=0.

jeff-li commented 4 years ago

Thanks for creating this PR! Is there any update regarding when this can be merged?

alysivji commented 4 years ago

Thanks for the ping. This slipped off my radar as other priorities came up.

I'm not a fan of the n+1 calls required for this feature to work properly. Adding a different abstraction on top or maybe doing a loop inside of the helper function would improve design.

I would need to take a closer look at what apispec is doing. I can take a look this weekend / early October (have some time devoted to Open Source during Hacktober) to see if I can come up with a better API.

jeff-li commented 4 years ago

thanks!