Opened 3 months ago

Closed 6 weeks ago

Last modified 6 weeks ago

#30275 closed defect (fixed)

Replace AssertionError with ValueError

Reported by: gh-mjungmath Owned by:
Priority: major Milestone: sage-9.2
Component: manifolds Keywords:
Cc: egourgoulhon, tscrim, mkoeppe Merged in:
Authors: Michael Jung Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: a20a6d2 (Commits) Commit: a20a6d29d125aae956111529df663be4b14943c7
Dependencies: Stopgaps:

Description (last modified by gh-mjungmath)

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 3 months ago by gh-mjungmath

  • Description modified (diff)

comment:2 Changed 2 months ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:3 Changed 8 weeks ago by gh-mjungmath

  • Branch set to u/gh-mjungmath/replace_assertionerror_with_valueerror

comment:4 Changed 8 weeks ago by gh-mjungmath

  • Authors set to Michael Jung
  • Commit set to 787bc4b5db9e06093df9a6b5fa2722de69427245
  • Status changed from new to needs_review

New commits:

787bc4bTrac #30275: AssertionError replaced

comment:5 Changed 8 weeks ago by gh-mjungmath

  • Dependencies #30181, #30261, #30239 deleted

comment:6 Changed 8 weeks ago by gh-mjungmath

  • Description modified (diff)

comment:7 Changed 8 weeks ago by tscrim

Why the change in algebras/commutative_dga.py?

comment:8 Changed 8 weeks ago by gh-mjungmath

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

Last edited 8 weeks ago by gh-mjungmath (previous) (diff)

comment:9 Changed 8 weeks ago by git

  • Commit changed from 787bc4b5db9e06093df9a6b5fa2722de69427245 to a20a6d29d125aae956111529df663be4b14943c7

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

a20a6d2Trac #30275: AssertionError replaced

comment:10 Changed 8 weeks ago by gh-mjungmath

There we go.

comment:11 Changed 7 weeks ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

Thanks. LGTM.

comment:12 follow-up: Changed 7 weeks ago by gh-mjungmath

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 7 weeks ago by tscrim

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 6 weeks ago by vbraun

  • Branch changed from u/gh-mjungmath/replace_assertionerror_with_valueerror to a20a6d29d125aae956111529df663be4b14943c7
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:15 Changed 6 weeks ago by mkoeppe

  • Milestone changed from sage-9.3 to sage-9.2
Note: See TracTickets for help on using tickets.