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

subclass Exception replace BaseException #997

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@s7v7nislands
Contributor

s7v7nislands commented Mar 21, 2015

All user-defined exceptions should also be derived from Exception
https://docs.python.org/3/library/exceptions.html?highlight=baseexception#base-classes

@berkerpeksag

This comment has been minimized.

Show comment
Hide comment
@berkerpeksag

berkerpeksag Mar 21, 2015

Collaborator

I think the BaseException usage was intentional: 8d453fb However, I'd at least change ConfigError to be derived from Exception.

Collaborator

berkerpeksag commented Mar 21, 2015

I think the BaseException usage was intentional: 8d453fb However, I'd at least change ConfigError to be derived from Exception.

@s7v7nislands

This comment has been minimized.

Show comment
Hide comment
@s7v7nislands

s7v7nislands Mar 21, 2015

Contributor

@berkerpeksag what's the reason change in
8d453fb
can you explain it?

Contributor

s7v7nislands commented Mar 21, 2015

@berkerpeksag what's the reason change in
8d453fb
can you explain it?

@berkerpeksag

This comment has been minimized.

Show comment
Hide comment
@berkerpeksag

berkerpeksag Mar 21, 2015

Collaborator

Well, I thought it was for to not catch HaltServer in gunicorn/arbiter.py explicitly, but I was wrong: https://github.com/benoitc/gunicorn/blob/master/gunicorn/arbiter.py#L200 Unless there is a reason to use BaseException, the patch LGTM.

Collaborator

berkerpeksag commented Mar 21, 2015

Well, I thought it was for to not catch HaltServer in gunicorn/arbiter.py explicitly, but I was wrong: https://github.com/benoitc/gunicorn/blob/master/gunicorn/arbiter.py#L200 Unless there is a reason to use BaseException, the patch LGTM.

@benoitc

This comment has been minimized.

Show comment
Hide comment
@benoitc

benoitc Mar 21, 2015

Owner

If I remember well the reason was to make sure it is not accidentally caught by code. There was also something in python 3 vs python2 but I can't recollect right now.

Owner

benoitc commented Mar 21, 2015

If I remember well the reason was to make sure it is not accidentally caught by code. There was also something in python 3 vs python2 but I can't recollect right now.

@benoitc

This comment has been minimized.

Show comment
Hide comment
@benoitc

benoitc Jul 7, 2015

Owner

So to be clear BaseException is used there in the same way it's used on low level modules in python. I would keep it for HaltServer then. Config I don't know but also don't really care about it. Does someone else think that change is needed?

Owner

benoitc commented Jul 7, 2015

So to be clear BaseException is used there in the same way it's used on low level modules in python. I would keep it for HaltServer then. Config I don't know but also don't really care about it. Does someone else think that change is needed?

@tilgovi

This comment has been minimized.

Show comment
Hide comment
@tilgovi

tilgovi Jul 7, 2015

Collaborator

This is the typical style. We should change it to be Exception unless there's a reason it won't work.

Collaborator

tilgovi commented Jul 7, 2015

This is the typical style. We should change it to be Exception unless there's a reason it won't work.

@berkerpeksag

This comment has been minimized.

Show comment
Hide comment
@berkerpeksag

berkerpeksag Jul 7, 2015

Collaborator

If I remember well the reason was to make sure it is not accidentally caught by code.

Actually, this is a good reason. +1 for keeping HaltServer as it is and changing other instances to inherit from Exception instead of BaseException.

Collaborator

berkerpeksag commented Jul 7, 2015

If I remember well the reason was to make sure it is not accidentally caught by code.

Actually, this is a good reason. +1 for keeping HaltServer as it is and changing other instances to inherit from Exception instead of BaseException.

@tilgovi

This comment has been minimized.

Show comment
Hide comment
@tilgovi

tilgovi Oct 8, 2015

Collaborator

👍 @s7v7nislands if you want to update the PR we can merge just the ConfigError change.

Collaborator

tilgovi commented Oct 8, 2015

👍 @s7v7nislands if you want to update the PR we can merge just the ConfigError change.

@tilgovi

This comment has been minimized.

Show comment
Hide comment
@tilgovi

tilgovi Oct 8, 2015

Collaborator

Maybe add a comment to the HaltServer that says that we don't inherit from Exception to avoid having it be caught by application code.

Collaborator

tilgovi commented Oct 8, 2015

Maybe add a comment to the HaltServer that says that we don't inherit from Exception to avoid having it be caught by application code.

@benoitc benoitc closed this in 44ac4e4 Oct 8, 2015

@benoitc

This comment has been minimized.

Show comment
Hide comment
@benoitc

benoitc Oct 8, 2015

Owner

@tilgovi good idea. just did it :)

Owner

benoitc commented Oct 8, 2015

@tilgovi good idea. just did it :)

fofanov pushed a commit to fofanov/gunicorn that referenced this pull request Mar 16, 2018

inherit from Exception where it needs to be
inherit from Exception where it needs to be and document why we use BaseException.

fix #997
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment