Skip to content
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

Don't use bare except: statements #27427

Closed
jdemeyer opened this issue Mar 5, 2019 · 26 comments
Closed

Don't use bare except: statements #27427

jdemeyer opened this issue Mar 5, 2019 · 26 comments

Comments

@jdemeyer
Copy link

jdemeyer commented Mar 5, 2019

It's well known that except: should be avoided because it catches unintended things like KeyboardInterrupt.

Component: misc

Author: Jeroen Demeyer

Branch: 367b0a1

Reviewer: Frédéric Chapoton

Issue created by migration from https://trac.sagemath.org/ticket/27427

@jdemeyer jdemeyer added this to the sage-8.7 milestone Mar 5, 2019
@jdemeyer

This comment has been minimized.

@jdemeyer jdemeyer changed the title Don't use base except: statements Don't use bare except: statements Mar 5, 2019
@jdemeyer
Copy link
Author

jdemeyer commented Mar 5, 2019

@jdemeyer
Copy link
Author

jdemeyer commented Mar 5, 2019

New commits:

cfcded7Fix bare "except:" statements

@jdemeyer
Copy link
Author

jdemeyer commented Mar 5, 2019

Commit: cfcded7

@fchapoton
Copy link
Contributor

comment:4

some failing doctests in inverse doctests, see patchbot

@tscrim
Copy link
Collaborator

tscrim commented Mar 5, 2019

comment:5

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

@fchapoton
Copy link
Contributor

comment:6

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

@jdemeyer
Copy link
Author

jdemeyer commented Mar 6, 2019

comment:7

Replying to @fchapoton:

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 7, 2019

Changed commit from cfcded7 to 0104d64

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 7, 2019

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

0104d64Fix bare "except:" statements

@tscrim
Copy link
Collaborator

tscrim commented Mar 7, 2019

comment:10

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.

@fchapoton
Copy link
Contributor

comment:11

doctests on inverse are still not fixed

@jdemeyer
Copy link
Author

jdemeyer commented Mar 7, 2019

comment:12

Sorry for the mess, I'm doing too many things at the same time.

@jdemeyer
Copy link
Author

jdemeyer commented Mar 7, 2019

comment:13

Replying to @fchapoton:

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)

@fchapoton
Copy link
Contributor

comment:14

pathcbots say that long test fails on 0104d64536e08abf61b4e

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 7, 2019

Changed commit from 0104d64 to 78ed09c

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 7, 2019

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

78ed09cFix bare "except:" statements

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 8, 2019

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

367b0a1Fix bare "except:" statements

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 8, 2019

Changed commit from 78ed09c to 367b0a1

@jdemeyer
Copy link
Author

jdemeyer commented Mar 8, 2019

comment:17

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").

@jdemeyer
Copy link
Author

jdemeyer commented Mar 8, 2019

comment:18

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).

@fchapoton
Copy link
Contributor

Reviewer: Frédéric Chapoton

@fchapoton
Copy link
Contributor

comment:19

ok, let it be..

@vbraun
Copy link
Member

vbraun commented Mar 11, 2019

Changed branch from u/jdemeyer/don_t_use_base_except__statements to 367b0a1

@soehms
Copy link
Member

soehms commented Mar 13, 2019

comment:21

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

@soehms
Copy link
Member

soehms commented Mar 13, 2019

Changed commit from 367b0a1 to none

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants