Opened 15 months ago

Closed 14 months ago

Last modified 14 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 15 months ago by gh-mjungmath

  • Description modified (diff)

comment:2 Changed 14 months ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:3 Changed 14 months ago by gh-mjungmath

  • Branch set to u/gh-mjungmath/replace_assertionerror_with_valueerror

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

  • Dependencies #30181, #30261, #30239 deleted

comment:6 Changed 14 months ago by gh-mjungmath

  • Description modified (diff)

comment:7 Changed 14 months ago by tscrim

Why the change in algebras/commutative_dga.py?

comment:8 Changed 14 months ago by gh-mjungmath

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

Last edited 14 months ago by gh-mjungmath (previous) (diff)

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

There we go.

comment:11 Changed 14 months ago by tscrim

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

Thanks. LGTM.

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

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