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

Black as a library import should check for running event loop before installing #2302

Closed
skrawcz opened this issue Jun 3, 2021 · 3 comments · Fixed by #2303
Closed

Black as a library import should check for running event loop before installing #2302

skrawcz opened this issue Jun 3, 2021 · 3 comments · Fixed by #2303
Assignees
Labels
S: accepted The changes in this design / enhancement issue have been accepted and can be implemented T: enhancement New feature or request

Comments

@skrawcz
Copy link

skrawcz commented Jun 3, 2021

# If our environment has uvloop installed lets use it
try:
import uvloop
uvloop.install()
except ImportError:
pass

See comments I left here 754eecf#r51686621

what is the issue?

We are using the black library in a system that already had an eventloop set. Importing black the library now clears the event loop settings we had. Impact to us is that we cannot use this latest version of black.

Suggestion

Black the library should not assume that it can create its own event loop -- instead it should check to see if a running event loop exists before calling uvloop.install().

Actually, I'd go further to ask why the library should be thinking about controlling an eventloop in the first place? Much like configuring loggers should not be set in libraries, event loops should not be set in libraries either?

@JelleZijlstra
Copy link
Collaborator

This also caused problems for us, though I haven't had time to investigate exactly what happened yet.

I suggest we should do uvloop.install only when Black is invoked from the command line, perhaps in patched_main(). This prevents side effects when Black is imported as a library.

@ichard26 ichard26 added the T: enhancement New feature or request label Jun 3, 2021
@ichard26 ichard26 added the S: accepted The changes in this design / enhancement issue have been accepted and can be implemented label Jun 3, 2021
@JelleZijlstra
Copy link
Collaborator

Note that Black doesn't have a supported library API yet (#779). However, we recognize that many people are already importing Black as a library and we shouldn't make the experience worse.

@cooperlees cooperlees self-assigned this Jun 3, 2021
@cooperlees
Copy link
Collaborator

Yup. This all makes sense. I'll get a PR up to move the loop install for black to patched_main()..

I think it still makes sense for blackd / primer where it is.

@ambv ambv closed this as completed in #2303 Jun 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S: accepted The changes in this design / enhancement issue have been accepted and can be implemented T: enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants