sagemath/sage

Move attrcall and friends from sage.misc.misc to new module sage.misc.call

Closed this issue · 25 comments

This will help with #29865

CC: @tscrim

Component: refactoring

Author: Matthias Koeppe

Branch/Commit: 6024ffd

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/29869

Description changed:

--- 
+++ 
@@ -1 +1 @@
-
+This will help with #29865

Commit: 907feeb

comment:3

The reimport in sage.misc.misc probably needs a deprecation?


New commits:

907feebMove attrcall and friends from sage.misc.misc to new module sage.misc.call

Author: Matthias Koeppe

Branch pushed to git repo; I updated commit sha1. New commits:

a5453bfFixup: Add src/sage/misc/call.py

Changed commit from 907feeb to a5453bf

Branch pushed to git repo; I updated commit sha1. New commits:

64c5701lazy_import from sage.misc.call with deprecation
65414f7Fix imports and one deprecation warning

Changed commit from a5453bf to 65414f7

comment:7

I think this is clean now, needs review

comment:8

You need to add the new file to

src/doc/en/reference/misc/index.rst

It is also missing the standard header information:

"""
Miscellaneous calling functions
"""

# Copyright...

Changed commit from 65414f7 to b9314d4

Branch pushed to git repo; I updated commit sha1. New commits:

b9314d4sage.misc.call: Add standard header information, add to reference manual

Reviewer: Travis Scrimshaw

comment:10

Thanks. If the patchbot comes back green, then this is a positive review.

comment:11

Well, there are all kinds of colors, but I think this is from code that was already there

comment:12

There is one real failure:

sage -t --long src/sage/modules/with_basis/indexed_element.pyx  # 1 doctest failed
comment:13
explain_pickle(x)
pg_dynamic_class = unpickle_global('sage.structure.dynamic_class', 'dynamic_class')
pg_CombinatorialFreeModuleElement = unpickle_global('sage.combinat.free_module', 'CombinatorialFreeModuleElement')
pg_getattr = unpickle_global('__builtin__', 'getattr')
pg_call_method = unpickle_global('sage.misc.misc', 'call_method')
pg_unreduce = unpickle_global('sage.structure.unique_representation', 'unreduce')
pg_Modules = unpickle_global('sage.categories.modules', 'Modules')
pg_IntegerRing = unpickle_global('sage.rings.integer_ring', 'IntegerRing')
pg = unpickle_instantiate(pg_IntegerRing, ())
si1 = pg_call_method(pg_unreduce(pg_Modules, (pg,), {'dispatch':False}), '_with_axiom', 'WithBasis')
si2 = unpickle_newobj(pg_dynamic_class('CombinatorialFreeModule_with_category.element_class', (pg_CombinatorialFreeModuleElement, pg_getattr(pg_call_method(si1, '_with_axiom', 'FiniteDimensional'), 'element_class')), None, None, None), ())
pg_CombinatorialFreeModule = unpickle_global('sage.combinat.free_module', 'CombinatorialFreeModule')
pg_FiniteEnumeratedSet = unpickle_global('sage.sets.finite_enumerated_set', 'FiniteEnumeratedSet')
pg_make_integer = unpickle_global('sage.rings.integer', 'make_integer')
unpickle_build(si2, (pg_unreduce(pg_CombinatorialFreeModule, (pg, pg_unreduce(pg_FiniteEnumeratedSet, (('x', 'y'),), {})), {'category':si1, 'prefix':'B'}), {'_monomial_coefficients':{'y':pg_make_integer('2'), 'x':pg_make_integer('2')}}))
si2

OK... I need some help here. Do I need to add register_unpickle_override?

comment:14

Yes, that is correct. You can add it in the old file to redirect to the new file. The other option (which has happened) is to simply drop the backwards compatibility and tell people to find a version of Sage during the deprecation period to load it then resave as a new pickle. IMO, I would rather do the former here since it is a 1-line fix.

Branch pushed to git repo; I updated commit sha1. New commits:

6024ffdsrc/sage/misc/call.py: register_unpickle_override for call_method

Changed commit from b9314d4 to 6024ffd

comment:16

Thanks. I had to add it to the new file though because sage.misc.misc is a dependency of sage.misc.persist.


New commits:

6024ffdsrc/sage/misc/call.py: register_unpickle_override for call_method
comment:17

I think it doesn't matter where it is at. I just want to see one more run of the patchbot before setting a positive review.

comment:19

Thanks!