#27427 closed defect (fixed)
Don't use bare except: statements
Reported by:  jdemeyer  Owned by:  

Priority:  major  Milestone:  sage8.7 
Component:  misc  Keywords:  
Cc:  Merged in:  
Authors:  Jeroen Demeyer  Reviewers:  Frédéric Chapoton 
Report Upstream:  N/A  Work issues:  
Branch:  367b0a1 (Commits)  Commit:  
Dependencies:  Stopgaps: 
Description (last modified by )
It's well known that except:
should be avoided because it catches unintended things like KeyboardInterrupt
.
Change History (21)
comment:1 Changed 19 months ago by
 Description modified (diff)
 Summary changed from Don't use base except: statements to Don't use bare except: statements
comment:2 Changed 19 months ago by
 Branch set to u/jdemeyer/don_t_use_base_except__statements
comment:3 Changed 19 months ago by
 Commit set to cfcded75417fa605021624a00868647eb82b0b79
 Status changed from new to needs_review
comment:4 Changed 19 months ago by
 Status changed from needs_review to needs_work
some failing doctests in inverse doctests, see patchbot
comment:5 Changed 19 months ago by
Seems like the failures are from the change in rings/valuation/valuation_space.py
.
comment:6 followup: ↓ 7 Changed 19 months ago by
This is because _test_inverse
in the same file is ignoring the NotImplementedError
cases only.
comment:7 in reply to: ↑ 6 Changed 19 months ago by
Replying to chapoton:
This is because
_test_inverse
in the same file is ignoring theNotImplementedError
cases only.
That's wrong: inverting a noninvertible element shouldn't raise NotImplementedError
.
comment:8 Changed 19 months ago by
 Commit changed from cfcded75417fa605021624a00868647eb82b0b79 to 0104d64536e08abf61b4ef2e070719517eb97f52
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
0104d64  Fix bare "except:" statements

comment:9 Changed 19 months ago by
 Status changed from needs_work to needs_review
comment:10 Changed 19 months ago by
I think the code in matrix_gps
should catch a ValueError
as the is_positive_definite()
will fail with one when the matrix (group) is defined over a field without an embedding into C.
comment:11 followup: ↓ 13 Changed 19 months ago by
 Status changed from needs_review to needs_work
doctests on inverse are still not fixed
comment:12 Changed 19 months ago by
Sorry for the mess, I'm doing too many things at the same time.
comment:13 in reply to: ↑ 11 Changed 19 months ago by
Replying to chapoton:
doctests on inverse are still not fixed
Are you sure? I ran all tests in src/sage/rings
and it worked fine.
(edit: never mind, I ran all tests without long
and the failing ones all have # long time
)
comment:14 Changed 19 months ago by
pathcbots say that long test fails on 0104d64536e08abf61b4e
comment:15 Changed 19 months ago by
 Commit changed from 0104d64536e08abf61b4ef2e070719517eb97f52 to 78ed09c8fa5e05383a45fd59f48a07145adfd8b5
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
78ed09c  Fix bare "except:" statements

comment:16 Changed 19 months ago by
 Commit changed from 78ed09c8fa5e05383a45fd59f48a07145adfd8b5 to 367b0a113e1dff335033b2fafa6efe015e68f517
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
367b0a1  Fix bare "except:" statements

comment:17 Changed 19 months ago by
 Status changed from needs_work to needs_review
I think that this fixes all comments. I decided to make some changes to the handling of matrix groups, printing "positive definite" when it is positive definite (previously, it only printed "non positive definite").
comment:18 Changed 19 months ago by
Note that the other places where I removed a try
/except
block was with code of the form
try: ... except: raise ...
In general, I don't like this code structure, I prefer the keep the original exception (which often contains more useful information).
comment:19 Changed 19 months ago by
 Reviewers set to Frédéric Chapoton
 Status changed from needs_review to positive_review
ok, let it be..
comment:20 Changed 19 months ago by
 Branch changed from u/jdemeyer/don_t_use_base_except__statements to 367b0a113e1dff335033b2fafa6efe015e68f517
 Resolution set to fixed
 Status changed from positive_review to closed
comment:21 Changed 19 months ago by
 Commit 367b0a113e1dff335033b2fafa6efe015e68f517 deleted
Thanks Jeroen, for correcting my mistakes! Thanks Travis and Frédéric for your help on that!
New commits:
Fix bare "except:" statements