Opened 2 years ago

Closed 22 months ago

Last modified 22 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, GitHub, GitLab) Commit: a20a6d29d125aae956111529df663be4b14943c7
Dependencies: Stopgaps:

Status badges

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 2 years ago by gh-mjungmath

  • Description modified (diff)

comment:2 Changed 22 months ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:3 Changed 22 months ago by gh-mjungmath

  • Branch set to u/gh-mjungmath/replace_assertionerror_with_valueerror

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

  • Dependencies #30181, #30261, #30239 deleted

comment:6 Changed 22 months ago by gh-mjungmath

  • Description modified (diff)

comment:7 Changed 22 months ago by tscrim

Why the change in algebras/commutative_dga.py?

comment:8 Changed 22 months ago by gh-mjungmath

That is not supposed to be there. Change it.

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

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

There we go.

comment:11 Changed 22 months ago by tscrim

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

Thanks. LGTM.

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

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