Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#30275 closed defect (fixed)

Replace AssertionError with ValueError

Reported by: Michael Jung Owned by:
Priority: major Milestone: sage-9.2
Component: manifolds Keywords:
Cc: Eric Gourgoulhon, Travis Scrimshaw, Matthias Köppe Merged in:
Authors: Michael Jung Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: a20a6d2 (Commits, GitHub, GitLab) Commit: a20a6d29d125aae956111529df663be4b14943c7
Dependencies: Stopgaps:

Status badges

Description (last modified by Michael Jung)

Turns out that an AssertionError is inapropriate to raise when immutable elements are tried to be changed (see here):

exception AssertionError

Raised when an assert statement fails.

since no assert statements are involved. A better choice would be a ValueError.

Change History (15)

comment:1 Changed 2 years ago by Michael Jung

Description: modified (diff)

comment:2 Changed 2 years ago by Matthias Köppe

Milestone: sage-9.2sage-9.3

comment:3 Changed 2 years ago by Michael Jung

Branch: u/gh-mjungmath/replace_assertionerror_with_valueerror

comment:4 Changed 2 years ago by Michael Jung

Authors: Michael Jung
Commit: 787bc4b5db9e06093df9a6b5fa2722de69427245
Status: newneeds_review

New commits:

787bc4bTrac #30275: AssertionError replaced

comment:5 Changed 2 years ago by Michael Jung

Dependencies: #30181, #30261, #30239

comment:6 Changed 2 years ago by Michael Jung

Description: modified (diff)

comment:7 Changed 2 years ago by Travis Scrimshaw

Why the change in algebras/commutative_dga.py?

comment:8 Changed 2 years ago by Michael Jung

That is not supposed to be there. I'll change it.

Last edited 2 years ago by Michael Jung (previous) (diff)

comment:9 Changed 2 years ago by git

Commit: 787bc4b5db9e06093df9a6b5fa2722de69427245a20a6d29d125aae956111529df663be4b14943c7

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

a20a6d2Trac #30275: AssertionError replaced

comment:10 Changed 2 years ago by Michael Jung

There we go.

comment:11 Changed 2 years ago by Travis Scrimshaw

Reviewers: Travis Scrimshaw
Status: needs_reviewpositive_review

Thanks. LGTM.

comment:12 Changed 2 years ago by Michael Jung

Thank you for the quick review.

What about the pyflakes errors? It has nothing to do with the ticket, but shouldn't that be polished at some point?

comment:13 in reply to:  12 Changed 2 years ago by Travis Scrimshaw

Replying to gh-mjungmath:

Thank you for the quick review.

No problem.

What about the pyflakes errors? It has nothing to do with the ticket, but shouldn't that be polished at some point?

Yes, but that is something for another ticket. You probably can take care of all of those at once for the manifolds folder.

comment:14 Changed 2 years ago by Volker Braun

Branch: u/gh-mjungmath/replace_assertionerror_with_valueerrora20a6d29d125aae956111529df663be4b14943c7
Resolution: fixed
Status: positive_reviewclosed

comment:15 Changed 2 years ago by Matthias Köppe

Milestone: sage-9.3sage-9.2
Note: See TracTickets for help on using tickets.