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

fix config of port and url #302

Merged
merged 3 commits into from Nov 5, 2022
Merged

fix config of port and url #302

merged 3 commits into from Nov 5, 2022

Conversation

kimhyun5u
Copy link
Contributor

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.

@netlify
Copy link

netlify bot commented Nov 3, 2022

Deploy Preview for robyn canceled.

Name Link
🔨 Latest commit 4d590a6
🔍 Latest deploy log https://app.netlify.com/sites/robyn/deploys/636636ac46a63d0008dd6441

@sansyrox
Copy link
Member

sansyrox commented Nov 3, 2022

@kimhyun5u , thank you for your PR. But doesn't robyn.env override the defaults the way it is?

@kimhyun5u
Copy link
Contributor Author

kimhyun5u commented Nov 3, 2022

@sansyrox , yes it is.
But, actually I learned about robyn on https://sansyrox.github.io/robyn/#/getting-started.
But there is no mention about robyn.env, so I spent lot of time to start robyn app that is set port and url each not 5000 and not 127.0.01.
That will make confused to users like me.
I think you would better mention about robyn.env on https://sansyrox.github.io/robyn/#/getting-started or change the method about configuring port and url

@sansyrox
Copy link
Member

sansyrox commented Nov 4, 2022

But there is no mention about robyn.env, so I spent lot of time to start robyn app that is set port and url each not 5000 and not 127.0.01. That will make confused to users like me.

Fair enough. Would you like to create a PR to update the documentation?

@kimhyun5u
Copy link
Contributor Author

kimhyun5u commented Nov 4, 2022

@sansyrox , I love to update the document.
But this current code ignores app.start() parameter(port, url).
So, I think it needs appropriate action like changing the code.

fix indenting style and add parameters type
@sansyrox
Copy link
Member

sansyrox commented Nov 4, 2022

@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 ROBYN_PORT=5002 python3 index.py when there is no robyn.env file then it will run it on PORT=5002.

How will the PR change the behaviour?

@kimhyun5u
Copy link
Contributor Author

@sansyrox , I think there are two issues on the current code.

First, the code,app.start(port=5000, url="0.0.0.0") , is not working.

The port and url is written in this code, but if i started this code by python3 app.py, the url of this app is set to 127.0.0.1.
Of course, if i start by ROBYN_URL=0.0.0.0 python3 index.py, it will be set to 0.0.0.0
Although it is working properly by another way, robyn should fix meaningfulness parameter.

Second, even though there is a way that work it right a way like ROBYN_URL=0.0.0.0 python3 index.py or creating robyn.enc, the docs is not mention about this.
Users who is the first time to use robyn would follow the docs like getting started. But the docs is not noticing about this, it makes confused them.

So robyn should offer that app.start(port=5000, url="0.0.0.0") is working properly. Or, If robyn wants it to work only as robyn.env, it should remove the parameters of app.start() to avoid user's misunderstanding.

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 robyn.env.

@sansyrox
Copy link
Member

sansyrox commented Nov 5, 2022

First, the code,app.start(port=5000, url="0.0.0.0") , is not working. The port and url is written in this code, but if i started this code by python3 app.py, the url of this app is set to 127.0.0.1.

Ah, my bad. I am surprised how it went unnoticed in the past two versions. Thank you! 😄

Everything makes complete sense. Thank you :D

Copy link
Member

@sansyrox sansyrox left a comment

Choose a reason for hiding this comment

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

Great work. Thank you for the PR. I just have a few comments.

Also, do you think we can add a test to ensure this doesn't happen again?

@@ -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):
Copy link
Member

Choose a reason for hiding this comment

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

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))

Copy link
Member

Choose a reason for hiding this comment

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

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/

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 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
@kimhyun5u
Copy link
Contributor Author

kimhyun5u commented Nov 5, 2022

@sansyrox , I think if robyn showed the port number and url that is used now, i wouldn't be confused.
So It might be better to print port and url on console like the other framework.

I'll have to think a bit about the testing.

Thank you for your dedication to OSS.

@sansyrox
Copy link
Member

sansyrox commented Nov 5, 2022

I think if robyn showed the port number and url that is used now, i wouldn't be confused. So It might be better to print port and url on console like the other framework.

Makes sense. I'll merge your PR now. I'll add it afterward. :D

Copy link
Member

@sansyrox sansyrox left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

@sansyrox sansyrox merged commit 29696f2 into sparckles:main Nov 5, 2022
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.

None yet

2 participants