Opened 19 months ago

Closed 19 months ago

Last modified 19 months ago

#27427 closed defect (fixed)

Don't use bare except: statements

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-8.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 jdemeyer)

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 jdemeyer

  • 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 jdemeyer

  • Branch set to u/jdemeyer/don_t_use_base_except__statements

comment:3 Changed 19 months ago by jdemeyer

  • Commit set to cfcded75417fa605021624a00868647eb82b0b79
  • Status changed from new to needs_review

New commits:

cfcded7Fix bare "except:" statements

comment:4 Changed 19 months ago by chapoton

  • Status changed from needs_review to needs_work

some failing doctests in inverse doctests, see patchbot

comment:5 Changed 19 months ago by tscrim

Seems like the failures are from the change in rings/valuation/valuation_space.py.

comment:6 follow-up: Changed 19 months ago by chapoton

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 jdemeyer

Replying to chapoton:

This is because _test_inverse in the same file is ignoring the NotImplementedError cases only.

That's wrong: inverting a non-invertible element shouldn't raise NotImplementedError.

comment:8 Changed 19 months ago by git

  • Commit changed from cfcded75417fa605021624a00868647eb82b0b79 to 0104d64536e08abf61b4ef2e070719517eb97f52

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

0104d64Fix bare "except:" statements

comment:9 Changed 19 months ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:10 Changed 19 months ago by tscrim

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 follow-up: Changed 19 months ago by chapoton

  • Status changed from needs_review to needs_work

doctests on inverse are still not fixed

comment:12 Changed 19 months ago by jdemeyer

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 jdemeyer

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)

Last edited 19 months ago by jdemeyer (previous) (diff)

comment:14 Changed 19 months ago by chapoton

pathcbots say that long test fails on 0104d64536e08abf61b4e

comment:15 Changed 19 months ago by git

  • Commit changed from 0104d64536e08abf61b4ef2e070719517eb97f52 to 78ed09c8fa5e05383a45fd59f48a07145adfd8b5

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

78ed09cFix bare "except:" statements

comment:16 Changed 19 months ago by git

  • Commit changed from 78ed09c8fa5e05383a45fd59f48a07145adfd8b5 to 367b0a113e1dff335033b2fafa6efe015e68f517

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

367b0a1Fix bare "except:" statements

comment:17 Changed 19 months ago by jdemeyer

  • 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 jdemeyer

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 chapoton

  • 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 vbraun

  • 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 soehms

  • Commit 367b0a113e1dff335033b2fafa6efe015e68f517 deleted

Thanks Jeroen, for correcting my mistakes! Thanks Travis and Frédéric for your help on that!

Note: See TracTickets for help on using tickets.