fix config of port and url #302
Conversation
✅ Deploy Preview for robyn canceled.
|
|
@kimhyun5u , thank you for your PR. But doesn't |
|
@sansyrox , yes it is. |
Fair enough. Would you like to create a PR to update the documentation? |
|
@sansyrox , I love to update the document. |
|
@kimhyun5u , I still don't understand the need for this. Robyn already allows you to set port via environment variables. e.g. if you run the command How will the PR change the behaviour? |
|
@sansyrox , I think there are two issues on the current code. First, the code, The port and url is written in this code, but if i started this code by Second, even though there is a way that work it right a way like So robyn should offer that Finally, the PR offer that the parameters of app.start() get the top priority in configuring the port and url and if there is no parameter in app.start, then look up |
Ah, my bad. I am surprised how it went unnoticed in the past two versions. Thank you! 😄 Everything makes complete sense. Thank you :D |
robyn/__init__.py
Outdated
| @@ -103,15 +103,17 @@ def startup_handler(self, handler: Callable) -> None: | |||
| def shutdown_handler(self, handler: Callable) -> None: | |||
| self._add_event_handler(Events.SHUTDOWN, handler) | |||
|
|
|||
| def start(self, url: str = "127.0.0.1", port: int = 5000): | |||
| def start(self, url: str | None = None, port: int | None = None): | |||
There was a problem hiding this comment.
can you make the add the default parameters like the original and then instead of
if url is None:
url = os.getenv("ROBYN_URL", "127.0.0.1")
if port is None:
port = int(os.getenv("ROBYN_PORT", "5000"))
we can do
url = os.getenv("ROBYN_URL", url)
port = int(os.getenv("ROBYN_PORT", port))
There was a problem hiding this comment.
Also, you will have to import from typing import Union instead of | as we are supporting python versions before Python 3.10 and | was introduced in 3.10/
There was a problem hiding this comment.
I think it is better changing the code to
url = os.getenv("ROBYN_URL", url)
port = int(os.getenv("ROBYN_PORT", port))
And remain the default parameters like the original.
fix the configure of port and url to look up robyn.env first
|
@sansyrox , I think if robyn showed the port number and url that is used now, i wouldn't be confused. I'll have to think a bit about the testing. Thank you for your dedication to OSS. |
Makes sense. I'll merge your PR now. I'll add it afterward. :D |
Description
This PR fixes an issue with configuring port and url. In #292, you change code to fix port and url each 500 and 127.0.01 when robyn.env doesn't exist.
So, I changed url and port that is the parameter of app.start() method to optional parameter.
Then, you can use app.start() without parameter, and you can use parameter port and url ignoring robyn.env.