Skip to content

Add server.url property in tests - #300

Merged
florimondmanca merged 2 commits into
encode:masterfrom
theruziev:server_url_test
Sep 1, 2019
Merged

florimondmanca merged 2 commits into
encode:masterfrom
theruziev:server_url_test

Conversation

@theruziev

@theruziev theruziev commented Aug 31, 2019 •

Copy link
Copy Markdown
Contributor

Fixes #288

@florimondmanca florimondmanca left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I checked out the code locally, looks all fine to me. Awesome work! Just left a tiny suggestion. :-)

Edit: I went ahead and edited the PR description with a reference to the issue, so it's automatically closed when we merge. 馃憤

Comment thread tests/conftest.py Outdated
@property
def url(self) -> URL:
protocol = "https" if self.config.is_ssl else "http"
return URL(f"{protocol}://{self.config.host}:{self.config.port}/")

@florimondmanca florimondmanca Aug 31, 2019 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just a nit: could we consider placing this property at the top of the class? The serve(), restart() and watch_restarts() methods are all kind of related in a non-immediately obvious way, so I think it's best to keep them together.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

@sethmlarson sethmlarson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What an awesome first contribution!!! Thanks so much :) Everything looks great to me. I'll leave this for you @florimondmanca to merge when ready.

@sethmlarson sethmlarson changed the title Add server.url property in tests #288 Add server.url property in tests Sep 1, 2019

@florimondmanca florimondmanca left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Fab! Thank you @theruziev!

@florimondmanca
florimondmanca merged commit e34d280 into encode:master Sep 1, 2019
@lovelydinosaur

Copy link
Copy Markdown
Member

Good stuff!
We could also accept a pull request adding this directly to uvicorn's API itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Add server.url property in tests

4 participants