Opened 2 years ago

Closed 23 months ago

#30281 closed enhancement (fixed)

remove __reduce__ method from Mutability

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

Status badges

Description (last modified by gh-mjungmath)

The class Mutability used to be a mixin class for mutable objects. However, not every class inheriting from it uses pickling and even then it must be overloaded manually in most cases anyway.

We suggest to remove that method from Mutability.

Change History (14)

comment:1 Changed 2 years ago by gh-mjungmath

  • Authors set to Michael Jung
  • Cc tscrim mkoeppe egourgoulhon added
  • Component changed from PLEASE CHANGE to misc
  • Description modified (diff)
  • Type changed from PLEASE CHANGE to enhancement

comment:2 Changed 2 years ago by gh-mjungmath

  • Description modified (diff)

comment:3 Changed 2 years ago by gh-mjungmath

  • Branch set to u/gh-mjungmath/remove___reduce___method_from_mutability

comment:4 Changed 2 years ago by gh-mjungmath

  • Commit set to f0230f0bccfb3ca11db0e77633530ced20ba0ba8
  • Status changed from new to needs_review

New commits:

f0230f0Trac #30281: __reduce__ method removed

comment:5 Changed 2 years ago by mkoeppe

Is this for a ticket where the existence of this method is a creating a problem?

comment:6 Changed 2 years ago by gh-mjungmath

Yes. See comment:10 in #30261 and all the following by Travis.

Last edited 2 years ago by gh-mjungmath (previous) (diff)

comment:7 Changed 2 years ago by gh-mjungmath

It seems that inheriting from Mutability causes a pickling test. Probably that is the reason why it is endowed with this method.

comment:8 follow-up: Changed 2 years ago by mkoeppe

Yes. When there is a __reduce__ method, then it is tested. The correct fix is to implement pickling for your class in #30261.

comment:9 in reply to: ↑ 8 Changed 2 years ago by gh-mjungmath

Replying to mkoeppe:

Yes. When there is a __reduce__ method, then it is tested. The correct fix is to implement pickling for your class in #30261.

This is exactly the reason why we remove __reduce__ from Mutability. But even then, pickling is tested (see #30280).

comment:10 Changed 2 years ago by tscrim

This __reduce__ method is bad because it it tries to unpickle anything as a Mutability object. So a Python class that inherits from this must then contend with this, which puts more of burden on the implementer. Perhaps there is a better way to do this, but it is not immediate to me how to do this...

comment:11 Changed 23 months ago by tscrim

  • Reviewers set to Travis Scrimshaw

I really think this should be removed. However, we need to make sure that when we pickle and unpickle an immutable object X it remains immutable.

comment:12 Changed 23 months ago by mkoeppe

Fine with me too

comment:13 Changed 23 months ago by mkoeppe

  • Reviewers changed from Travis Scrimshaw to Travis Scrimshaw, Matthias Koeppe
  • Status changed from needs_review to positive_review

comment:14 Changed 23 months ago by vbraun

  • Branch changed from u/gh-mjungmath/remove___reduce___method_from_mutability to f0230f0bccfb3ca11db0e77633530ced20ba0ba8
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.