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

Protocol and port control #201

Closed
wants to merge 2 commits into from
Closed

Protocol and port control #201

wants to merge 2 commits into from

Conversation

vpet98
Copy link
Contributor

@vpet98 vpet98 commented Feb 10, 2023

Based on issue #176.

I added a parameter in the add_explorer_view, so that it is possible to define the protocol (e.g. https instead of http) in the specification url. In such case, the port of the application also needs to be given in the route_url function of pyramid Request object, that's why the added parameter is a tuple of two values.

@zupo
Copy link
Collaborator

zupo commented Feb 10, 2023

Looks good! Can you add a test as well please?

@vpet98
Copy link
Contributor Author

vpet98 commented Feb 11, 2023

Thank you! I added a test for this case.

In order to make the test work I had also to add a host parameter in the add_explorer_view function. I am not sure why it would fail without that parameter, maybe it has to do with the DummyRequest object. If there is a way to avoid this, please tell me so that I can omit this redundant code.

Either way, I believe the user doesn't need to specify the host parameter when calling pyramid_openapi3_add_explorer to solve the problem described at issue #176. They only need to specify protocol and port.

@jschaeff
Copy link

jschaeff commented Mar 1, 2023

Hi @zupo, thank you for taking time to review this PR.

Can you tell roughly when it could hit the next release ?

Cheers

@zupo
Copy link
Collaborator

zupo commented Mar 1, 2023

I first need to figure out why all tests are failing.

@zupo zupo mentioned this pull request Apr 6, 2023
@zupo
Copy link
Collaborator

zupo commented Apr 6, 2023

I had to rewrite this PR a bit: #207

@zupo zupo closed this Apr 6, 2023
@zupo
Copy link
Collaborator

zupo commented Apr 6, 2023

In order to make the test work I had also to add a host parameter in the add_explorer_view function. I am not sure why it would fail without that parameter, maybe it has to do with the DummyRequest object. If there is a way to avoid this, please tell me so that I can omit this redundant code.

This problem only seems to affect the tests so, I just fixed the tests instead of adding a new host parameter.

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.

3 participants