Opened 6 years ago

Closed 6 years ago

#15026 closed defect (fixed)

Fix wrong except clauses

Reported by: jdemeyer Owned by:
Priority: minor Milestone: sage-5.12
Component: misc Keywords:
Cc: Merged in: sage-5.12.beta3
Authors: Jeroen Demeyer Reviewers: Punarbasu Purkayastha
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

A few places in Sage have except clauses like

except ValueError, TypeError:

but this is interpreted by Python as

except ValueError as TypeError:

while it should be

except (ValueError, TypeError):

Attachments (1)

15026_except_tuple.patch (7.3 KB) - added by jdemeyer 6 years ago.

Download all attachments as: .zip

Change History (6)

Changed 6 years ago by jdemeyer

comment:1 Changed 6 years ago by jdemeyer

  • Status changed from new to needs_review

comment:2 follow-up: Changed 6 years ago by ppurka

Is it clear in cases like this:

5181	 	                    except AttributeError, TypeError:

that the TypeError never occurs?

comment:3 in reply to: ↑ 2 Changed 6 years ago by jdemeyer

Replying to ppurka:

Is it clear in cases like this:

5181	 	                    except AttributeError, TypeError:

that the TypeError never occurs?

I thought about this but I couldn't see any way how alpha.galois_conjugates(QQbar) could produce a TypeError. Note that

except AttributeError, TypeError:

and

except AttributeError:

really do the same thing, so I'm not changing the functionality of this line of code.

comment:4 Changed 6 years ago by ppurka

  • Reviewers set to Punarbasu Purkayastha
  • Status changed from needs_review to positive_review

It is true that the code is functionally unchanged. But the way it is written makes me think that the original author intended to catch both AttributeError and TypeError. This clearly won't be caught in doctests since it has probably never been doctested when, if at all, TypeError is caught.

Anyway, it has been over two years since the code is present and no one has complained. So, a positive review then. :)

comment:5 Changed 6 years ago by jdemeyer

  • Merged in set to sage-5.12.beta3
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.