In roundup/exceptions.py there are some exceptions defined which are derived 
from BaseException. I have a use case where I have to handle those exceptions 
but I can't because they are using BaseException directly. I would have to 
catch it with "except BaseException:" which would result in also catching 
signals like keyboard interrupts or system exits).

I would propose to make a common exception class where all other custom Roundup 
exceptions are derived from. It is also done like this in the 
roundup/cgi/exceptions.py exceptions.

In addition I think we can derive the Roundup base exception class from 
"Exception" and not "BaseException" as the latter one should be used for built-
in exceptions and not user-defined exceptions.


roundup/exceptions.py

class RoundupException(Exception):
    pass

class Unauthorised(RoundupException):
    pass

class Reject(RoundupException):
    pass

...


If we align on this we could also go though the exceptions in 
roundup/cgi/exceptions.py and check for those concepts. We can import the 
RoundupException from roundup/exceptions.py and derive the exceptions for cgi.

Hi Robert:

In message <1585121254.8.0.412971837801.issue2551083@roundup.psfhosted.org>,
Robert Klonner writes:
>In roundup/exceptions.py there are some exceptions defined which are derived 
>from BaseException. I have a use case where I have to handle those exceptions 
>but I can't because they are using BaseException directly. I would have to 
>catch it with "except BaseException:" which would result in also catching 
>signals like keyboard interrupts or system exits).

IIRC BaseException was the way it was done in earlier 2.2 pythons. The
code base is showing its age.

I am interested in your use case. AFAIK you can always:

  from exceptions import *

then use

  try:
     ...
  except (exception1, exception2, exception3 ...):

(I may be off on the syntax but you get the idea). Then list each
exception you want to catch. Clumsy but workable.  However if a new
exception is added you will need to change your code 8-(.

I see Ralf is already on this ticket. Ralf are you working it?

I think (as recommended) a:

  class RoundupException(Exception):
     pass

and substituting BaseException/Exception in all class definitions with
RoundupException makes sense and follows recommended exception
hierarchy guidelines.

Does anybody see a problem with this? Any cases where this change may
break something?

I would like to get this in the 2.0 beta release (was planning on
starting the release sequence this weekend). Robert I assume you can
test it for your use case?

Hi Robert,

Ralf and I are discussing what need to change off list.

Can you describe your use case so we can be sure that all exceptions
(base roundup, email, cgi, tal, detector) that we are modifying will
meet your needs.

Thanks

-- rouilj

I think this is the new exception hierarchy that Ralf and I worked out.

+ indicates new exception class
* indicates exception class needs to be changed

Inheritence by indentation:

RoundupException+
   RejectBase+
      Reject*
         RejectRaw
   RoundupCGIException+
      HTTPException*
         Redirect
         NotFound
         NotModified
         PreconditionFailed
   DetectorError*
   SendFile*
   SendStaticFile*
   SeriousError*
   
   NoTemplate*
   Unauthorised* (2 instances)

   ConfigurationError*

   LoginError*
   Reject*

   TrackerError*

   MailUsageHelp*
   Unauthorized*
   IgnoreMessage*
   
Exception
   SysCallError - defined in except block on ssl import failure
   PoSyntaxError
   
BaseException
   SvcShutdown - server should exit on this
   
Exceptions defined under:

 roundup/cgi/TAL/...
 roundup/cgi/PageTemplates/...

stay as they are. These are inherited files into roundup.

Richard will this work for your use case?

Ralf does this look right? If so I'll try to do them this weekend.

-- rouilj

On Thu, Apr 02, 2020 at 09:13:48PM +0000, John Rouillard wrote:
> 
> John Rouillard added the comment:
> 
> I think this is the new exception hierarchy that Ralf and I worked out.
> 
> + indicates new exception class
> * indicates exception class needs to be changed
> 
> Inheritence by indentation:

Shouldn't RoundupException inherit from Exception?

Otherwise everything looks fine to me, thanks for your effort!

Ralf

> RoundupException+
>    RejectBase+
>       Reject*
>          RejectRaw
>    RoundupCGIException+
>       HTTPException*
>          Redirect
>          NotFound
>          NotModified
>          PreconditionFailed
>    DetectorError*
>    SendFile*
>    SendStaticFile*
>    SeriousError*
>    
>    NoTemplate*
>    Unauthorised* (2 instances)
> 
>    ConfigurationError*
> 
>    LoginError*
>    Reject*
> 
>    TrackerError*
> 
>    MailUsageHelp*
>    Unauthorized*
>    IgnoreMessage*
>    
> Exception
>    SysCallError - defined in except block on ssl import failure
>    PoSyntaxError
>    
> BaseException
>    SvcShutdown - server should exit on this
>    
> Exceptions defined under:
> 
>  roundup/cgi/TAL/...
>  roundup/cgi/PageTemplates/...
> 
> stay as they are. These are inherited files into roundup.

Ralf Said:

> Shouldn't RoundupException inherit from Exception?

Yes.

I have the code changes done. Running tests locally to see if
I broke anything. Sadly exception handling is not well covered
by the test suite, so I could break something silently.

Haven't heard back from Robert, hopefully this meets his needs.

Committed as rev:6121:c177e7128dc9 lets see what CI has to say.

Sigh, fixed in: rev6125:38fbfbb24cbd and  rev 6126:15c68580578f
and rev 6127:80915fd9ad24

Closing since we haven't gotten any feedback.