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

all except BaseException should be except Exception #2444

Open
hellysmile opened this issue Oct 30, 2017 · 6 comments
Open

all except BaseException should be except Exception #2444

hellysmile opened this issue Oct 30, 2017 · 6 comments
Labels

Comments

@hellysmile
Copy link
Member

Long story short

#2383 fixed flake8 blind exception, but later for example #2441 used except BaseException

It should be fixed separately in 2.3 and master branches

Expected behaviour

everywhere should be except Exception

Actual behaviour

Some parts of code uses except Exception, some except BaseException

@hellysmile hellysmile added the good first issue Good for newcomers label Oct 30, 2017
@hellysmile hellysmile changed the title all except BaseException should be except Exception all except BaseException should be except Exception Oct 30, 2017
@fafhrd91
Copy link
Member

fafhrd91 commented Oct 30, 2017

For 2441 BaseException makes sense, especially because it just re-raise exception

@socketpair
Copy link
Contributor

@asvetlov it is actual again. We need a linter.

See

except BaseException:
for example. Exception in close() prevent, for example, exitting from the application.

@socketpair socketpair reopened this Jan 4, 2019
@asvetlov
Copy link
Member

asvetlov commented Jan 4, 2019

No.
The code catches an exception, closes the response and reraises the exception again.
It should work pretty fine with CancelledError, isn't it?

Exception in close() unwinds the stack anyway.

@socketpair
Copy link
Contributor

Yes, unwinds, but exception in .close() will REPLACE, say, SystemExit exception, does not it ?

@asvetlov
Copy link
Member

Chicken-egg problem.
I have a feeling that a response should be closed in case of exception.
The question is: how should look like an exception in finalizer.
@socketpair do you have a suggestion?

@socketpair
Copy link
Contributor

  1. We should track only except Exception (and CancelledError also, if applicable).
  2. AFAIK, calling close() - just an GC optimisation. In good code, GC should kill such items.
  3. Another option - try...except...warning+ignore wrapper over close().

There are plenty of exceptions, which are not errors actually. It's wrong design. Like Stopiteration,systemexit,Cancellederror....We should close something only by ERROR. If someone raises such exceptions it should understand what will happen.

That's my opinion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants