-
-
Notifications
You must be signed in to change notification settings - Fork 258
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
Add help messages and simplify 'dev' option #128
Conversation
✔️ Deploy Preview for robyn canceled. 🔨 Explore the source changes: 4e4f8a0 🔍 Inspect the deploy log: https://app.netlify.com/sites/robyn/deploys/61b49b44b6903f0007593bc3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes look good but I have a question with the contribution.
dest="dev", | ||
action="store_true", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for contributing. But can you tell me what these flags do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! So, the change here is removing the dev
in the arguments and add in the options. The reason is that no one will ever do --dev=false
as it's the default.
This change just makes --dev
standalone possible. The action
argument on the highlighted above makes it store a True
value on the dev
variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, alright. Makes sense. Thank you! :D
Workers are the Whereas is the processes are the sharing the listening TCP socket across multiple python processes. So, you can imagine having multiple runtimes running in parallel. This is still experimental to see if they add any performance benefits or not. That's why the defaults are set to 1 at the moment. |
This is a great suggestion. I didn't know about this library. I will integrate this in the coming releases. :D |
I see. On |
Thanks for the feedback. Will do 🖖 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🚀
This PR adds the help messages (my own choice of words, but feel free to modify).
What's the difference between processes and workers here? I didn't inspect the code yet, but in the ASGI context it doesn't make sense.
Also, I'd recommend for us to use Click. It will make the writing of the CLI more pleasant.