Opened 4 months ago

Closed 3 months ago

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

  • Description modified (diff)

comment:2 Changed 3 months ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:3 Changed 3 months ago by gh-mjungmath

  • Branch set to u/gh-mjungmath/replace_assertionerror_with_valueerror

comment:4 Changed 3 months 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 3 months ago by gh-mjungmath

  • Dependencies #30181, #30261, #30239 deleted

comment:6 Changed 3 months ago by gh-mjungmath

  • Description modified (diff)

comment:7 Changed 3 months ago by tscrim

Why the change in algebras/commutative_dga.py?

comment:8 Changed 3 months ago by gh-mjungmath

That is not supposed to be there. Change it.

Version 0, edited 3 months ago by gh-mjungmath (next)

comment:9 Changed 3 months 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 3 months ago by gh-mjungmath

There we go.

comment:11 Changed 3 months ago by tscrim

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

Thanks. LGTM.

comment:12 follow-up: Changed 3 months 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 3 months 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 3 months 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 3 months ago by mkoeppe

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