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

docs: reorder README sections in order of importance #140

Merged
merged 1 commit into from
Oct 29, 2022

Conversation

empicano
Copy link
Owner

Hi Frederik!

Again with the README 😄 I tweaked a few little things, and reordered the sections, among others to move the Note for Windows users higher, as there are quite a few people who seem to overlook this. (Btw. is there anything that speaks against implementing the switch to the SelectorEventLoop directly in the code? I guess changing the loop silently is not a very nice pattern.)

~ Felix

@JonathanPlasse
Copy link
Collaborator

Why delete the requirement section?

@empicano
Copy link
Owner Author

I didn't delete it, I just merged it with the installation section 😉

@frederikaalund
Copy link
Collaborator

Good idea to move the "Note for Windows users" section up. 👍 We get a frequent stream of new issues due to that.

(Btw. is there anything that speaks against implementing the switch to the SelectorEventLoop directly in the code? I guess changing the loop silently is not a very nice pattern.)

Technically, there is no barrier. :) It's just intrusive to switch the loop like that IMHO. What we could do, is to issue a warning if the user attempts to use the library with a non-supported event loop. In that warning, we can instruct the user to switch the event loop. I weldcome a PR with that change. 👍

Back to this PR itself. LGTM. Thanks!

~Frederik

@frederikaalund frederikaalund merged commit 7ab7cc6 into master Oct 29, 2022
@empicano empicano deleted the reorder-readme branch October 29, 2022 13:58
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.

3 participants