Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed invalid methods error in falcon 3 #32

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

vuonglv1612
Copy link

@vuonglv1612 vuonglv1612 commented Jun 13, 2021

From falcon 3+, some methods was added which aren't in OpenAPI v3 valid methods.
We can filter invalid methods before add to mapping

Ref: #31

@codecov-commenter
Copy link

codecov-commenter commented Jun 13, 2021

Codecov Report

Merging #32 (e81e569) into master (a5e665e) will decrease coverage by 2.12%.
The diff coverage is 100.00%.

❗ Current head e81e569 differs from pull request most recent head 13734f0. Consider uploading reports for the commit 13734f0 to get more accurate results

Impacted file tree graph

@@             Coverage Diff             @@
##            master      #32      +/-   ##
===========================================
- Coverage   100.00%   97.87%   -2.13%     
===========================================
  Files            3        3              
  Lines           40       47       +7     
===========================================
+ Hits            40       46       +6     
- Misses           0        1       +1     
Impacted Files Coverage Δ
falcon_apispec/falcon_plugin.py 97.72% <100.00%> (-2.28%) ⬇️

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...13734f0. Read the comment docs.

@@ -28,6 +35,8 @@ def _generate_resource_uri_mapping(app):
for method_name, method_handler in route.method_map.items():
if method_handler.__dict__.get("__module__") == "falcon.responders":

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one needs to be fixed as well, because I'll not work with falcon==3.0, neither.

Suggested change
if method_handler.__dict__.get("__module__") == "falcon.responders":
if method_handler.__module__ == "falcon.responders":

Copy link

@Javlopez Javlopez Dec 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @aslantar I'm using this awesome plugin, but I really would like to use it with falcon3, can I send these changes again in another PR - branch?, I've tested and seems it is working fine with falcon3, what do you think?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @aslantar, I've been busy lately so I missed your review.
Thanks @Javlopez, please help me fix this problem, I don't have enough time to continue to maintain this PR

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Javlopez, sure. I'd love to see the changes integrated.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @aslantar the PR is here #34 please take a look

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Javlopez why do you need a separate PR with the same changes, instead of assisting @vuonglv1612 to get this one done?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hello, sorry I didn't see this before, well just I found a way to send eveything in one PR, but you are right I can send my changes to @vuonglv1612 repo, for me is ok, I gonna send them today later

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @aslantar I've opened a PR for in the fork vuonglv1612#1 just with the missing changes, @vuonglv1612 can you take a look please?

Best Regards

Add missing falcon responders
@Javlopez
Copy link

hey @aslantar when you have time could you please merge this PR? thank you so much

@aslantar
Copy link

hi @Javlopez, I approved already, but I'm not eligible to merge. @alysivji can you help?

@Javlopez
Copy link

Javlopez commented Mar 8, 2022

Hi @alysivji can you help us to merge the PR?, thank you so much

@adammkelly
Copy link

@alysivji Can this be merged?

@AndreasBBS
Copy link

@alysivji It's been basically an year since the solution to this critical bug has been provided. Any reason the PR is not being merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants