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

Add help messages and simplify 'dev' option #128

Merged
merged 1 commit into from
Dec 11, 2021
Merged

Add help messages and simplify 'dev' option #128

merged 1 commit into from
Dec 11, 2021

Conversation

Kludex
Copy link
Contributor

@Kludex Kludex commented Dec 11, 2021

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.

@netlify
Copy link

netlify bot commented Dec 11, 2021

✔️ Deploy Preview for robyn canceled.

🔨 Explore the source changes: 4e4f8a0

🔍 Inspect the deploy log: https://app.netlify.com/sites/robyn/deploys/61b49b44b6903f0007593bc3

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.

These changes look good but I have a question with the contribution.

Comment on lines +25 to +26
dest="dev",
action="store_true",
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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

@sansyrox
Copy link
Member

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.

Workers are the tokios way of implementing concurrency. This is mostly threads.

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.

@sansyrox
Copy link
Member

Also, I'd recommend for us to use Click.

This is a great suggestion. I didn't know about this library. I will integrate this in the coming releases. :D

@Kludex
Copy link
Contributor Author

Kludex commented Dec 11, 2021

I see. On uvicorn workers and processes are analogous. We probably want to mention that somewhere at some point. 👍

@sansyrox
Copy link
Member

I see. On uvicorn workers and processes are analogous. We probably want to mention that somewhere at some point. 👍

Thanks for the feedback. Will do 🖖

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 907adf4 into sparckles:main Dec 11, 2021
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.

2 participants