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

Change sys.exit to Raise. #2440

Merged
merged 4 commits into from
Aug 24, 2021
Merged

Change sys.exit to Raise. #2440

merged 4 commits into from
Aug 24, 2021

Conversation

erykoff
Copy link
Contributor

@erykoff erykoff commented Aug 23, 2021

The fix for #1688 in #1761 breaks help("modules") introspection and also leads to unhappy results when inadvertently importing blackd from python. E.g.:

$ python
Python 3.8.10 | packaged by conda-forge | (default, May 11 2021, 07:01:05) 
[GCC 9.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> help('modules')

Please wait a moment while I gather a list of all available modules...

aiohttp dependency is not installed: No module named 'aiohttp'. Please re-install black with the '[d]' extra install to obtain aiohttp_cors: `pip install black[d]`
$

(Notice that python has been exited).

This PR changes the sys.exit(-1) in the import code to a proper Raise.

src/blackd/__init__.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@ichard26 ichard26 left a comment

Choose a reason for hiding this comment

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

Ah yeah this makes sense. Thanks for the PR!

@cooperlees cooperlees requested a review from zsol August 24, 2021 03:55
Copy link
Collaborator

@zsol zsol left a comment

Choose a reason for hiding this comment

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

Lgtm, just one minor comment

src/blackd/__init__.py Outdated Show resolved Hide resolved
@ichard26
Copy link
Collaborator

Not waiting for all of primer since the chance of them uncovering an issue here is extremely low.

@ichard26 ichard26 merged commit 0969ca4 into psf:main Aug 24, 2021
@ichard26
Copy link
Collaborator

Hey thanks for your contribution! This project is only possible by contributions like these 🖤. You're awesome, @erykoff, never really thought about how a sys.exit would be problematic on import (not sure why, but eh who cares :p). If you have any suggestions about your contributing experience here, please let us via #2238, thank you once again!

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.

4 participants