Skip to content

Conversation

@Angeluz-07
Copy link
Contributor

@Angeluz-07 Angeluz-07 commented Apr 20, 2020

Fixes #47



def add_proxy_fix_to_serve_swagger_over_https(app: Flask):
from werkzeug.middleware.proxy_fix import ProxyFix
Copy link
Contributor

Choose a reason for hiding this comment

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

I would call this function add_fixers and in the commit then I would say

Add ProxyFix to serve swagger over HTTPs.

This is supposed to be registered in the CHANGELOG and provide this node.
You can leave the logged message because it serves to explain what this particular fix is for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with the commit message change. But I don't agree with the function name "add_fixers". It seems to general for me. This is a fix for a very specific problem. If later we found we need more fixers, it'll be ok to change the name to "add_fixers". But for now I think we are ok leaving the name of the function specific.

Copy link
Contributor

@EliuX EliuX Apr 21, 2020

Choose a reason for hiding this comment

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

Ok, but that function explains reasons and not what that actual function does. If you want to explain the reasons in a verbose way, better use a logger. But I understand your point on trying to be more specific, so I would name for instance use_werkzeug_proxy_fix, but whatever name with that idea in mind would do for me.

@Angeluz-07 Angeluz-07 force-pushed the fix/swagger-not-working-in-https#47 branch from 3269240 to d315d5b Compare April 21, 2020 18:58
@Angeluz-07 Angeluz-07 merged commit e0e683b into master Apr 21, 2020
@EliuX EliuX deleted the fix/swagger-not-working-in-https#47 branch April 21, 2020 19:21
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.

Https is not working for main page

3 participants