Opened 4 years ago

Closed 4 years ago

#22796 closed enhancement (fixed)

Remove deprecated_callable_import

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-8.0
Component: misc Keywords:
Cc: vdelecroix Merged in:
Authors: Jeroen Demeyer Reviewers: Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: 0ac7893 (Commits) Commit: 0ac7893146826afa62ffe437868e8c5d6d313c17
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

The functionality of deprecated_callable_import can be achieved in a simpler and more general way by a lazy import with deprecation.

So we remove deprecated_callable_import in this ticket and remove existing uses coming from #17034, #17867, #19096, #19315. Only one deprecation from #20908 remains.

Change History (24)

comment:1 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:2 Changed 4 years ago by jdemeyer

  • Description modified (diff)
  • Summary changed from Deprecate deprecated_callable_import to Remove deprecated_callable_import

comment:3 Changed 4 years ago by jdemeyer

  • Branch set to u/jdemeyer/deprecate_deprecated_callable_import

comment:4 Changed 4 years ago by git

  • Commit set to 4b90c163b2d85c5fd6a661ef8308471ddf2cd6bd

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

4b90c16Remove deprecated_callable_import

comment:5 Changed 4 years ago by jdemeyer

  • Cc vdelecroix added
  • Status changed from new to needs_review

comment:6 follow-up: Changed 4 years ago by vdelecroix

To make lazy_import invisible, I think the following is cleaner

from sage.misc.lazy_import import lazy_import
lazy_import('sage.some.package', 'some_function', deprecation=666)
del lazy_import

comment:7 in reply to: ↑ 6 Changed 4 years ago by jdemeyer

Replying to vdelecroix:

I think the following is cleaner

I think that import lazy_import as _lazy_import is better, because there are less chances for mistakes (one could easily forget to add the del lazy_import or accidentally remove that statement).

comment:8 Changed 4 years ago by dimpase

  • Reviewers set to Dima Pasechnik
  • Status changed from needs_review to positive_review

This looks good to me.

comment:9 Changed 4 years ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict...

comment:10 Changed 4 years ago by dimpase

it does merge cleanly in 8.0.beta5, thus I have no idea where the merge conflict comes from.

comment:11 Changed 4 years ago by dimpase

  • Status changed from needs_work to positive_review

comment:12 Changed 4 years ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict... Obviously the merge conflict comes from stuff in beta6, duh.

comment:13 Changed 4 years ago by dimpase

  • Branch changed from u/jdemeyer/deprecate_deprecated_callable_import to u/dimpase/t22796
  • Commit changed from 4b90c163b2d85c5fd6a661ef8308471ddf2cd6bd to 18e9a467945579b5fc907de02a46bef4eb888f0f
  • Status changed from needs_work to positive_review

it was weird seeing "Merge conflict" against a branch that was not public yet :-) anyhow, it's a trivial merge, one chunk was to be deleted.

comment:14 Changed 4 years ago by vbraun

  • Status changed from positive_review to needs_work
sage -t --long --warn-long 72.0 src/sage/combinat/designs/design_catalog.py
**********************************************************************
File "src/sage/combinat/designs/design_catalog.py", line 77, in sage.combinat.designs.design_catalog
Failed example:
    'absolute_import' in dir(designs) or 'deprecated_callable_import' in dir(designs)
Expected:
    False
Got:
    True
**********************************************************************
1 item had failures:
   1 of   3 in sage.combinat.designs.design_catalog
    [2 tests, 1 failure, 0.27 s]
----------------------------------------------------------------------
sage -t --long --warn-long 72.0 src/sage/combinat/designs/design_catalog.py  # 1 doctest failed
----------------------------------------------------------------------
Total time for all tests: 0.3 seconds
    cpu time: 0.3 seconds
    cumulative wall time: 0.3 seconds

comment:15 Changed 4 years ago by dimpase

should the following be applied:

--- a/src/sage/combinat/designs/design_catalog.py
+++ b/src/sage/combinat/designs/design_catalog.py
@@ -74,7 +74,7 @@ REFERENCES:
 
 TESTS::
 
-    sage: 'absolute_import' in dir(designs) or 'deprecated_callable_import' in dir(designs)
+    sage: 'deprecated_callable_import' in dir(designs)
     False
 """
 from __future__ import absolute_import

(I don't get the absolute_import part in this test---surely absolute_import is all over the place; removing it from the test makes the test pass)

comment:16 Changed 4 years ago by git

  • Commit changed from 18e9a467945579b5fc907de02a46bef4eb888f0f to e258a2053f99656bc3ee079f796758039d994750

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

179ac5ecleanup of Griesmer bound, docs and examples
bb33393Merge branch 'u/dimpase/codesizefix' of trac.sagemath.org:sage into codesizefix
224b1beRemove rename_keyword deprecation warning (6 years old)
1ee9d38Basic parameter sanity check on more bounds
4b0a57eMore doc-string fixes
5d2ee22Merge branch 'u/jsrn/codesizefix' of trac.sagemath.org:sage into codesizefix
e793a92added field_based and used it whenever needed
228451dadjusted the top docs to talk about codes not only over fields
e398f4cMerge branch 'u/dimpase/codesizefix' of trac.sagemath.org:sage into devbeta6
e258a20remove obsolete test

comment:17 Changed 4 years ago by dimpase

oops, wrong branch

comment:18 Changed 4 years ago by git

  • Commit changed from e258a2053f99656bc3ee079f796758039d994750 to f92d174b5e9890f57b14aeb9552cdb1399b268d4

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

f92d174remove obsolete test

comment:19 follow-up: Changed 4 years ago by dimpase

  • Status changed from needs_work to needs_review

the last commit removes an obsolete test - there is no deprecated_callable_import anywhere any more anyway.

comment:20 in reply to: ↑ 19 Changed 4 years ago by jdemeyer

Replying to dimpase:

the last commit removes an obsolete test - there is no deprecated_callable_import anywhere any more anyway.

Well, the test was about absolute_import and deprecated_callable_import. You only fixed the latter, I now fixed the former too.

comment:21 Changed 4 years ago by jdemeyer

  • Branch changed from u/dimpase/t22796 to u/jdemeyer/t22796

comment:22 Changed 4 years ago by dimpase

  • Commit changed from f92d174b5e9890f57b14aeb9552cdb1399b268d4 to 0ac7893146826afa62ffe437868e8c5d6d313c17

Sage imports drive me mad elsewhere too: https://trac.sagemath.org/ticket/22799#comment:90 :-)


New commits:

0ac7893Remove absolute_import from catalog

comment:23 Changed 4 years ago by dimpase

  • Status changed from needs_review to positive_review

comment:24 Changed 4 years ago by vbraun

  • Branch changed from u/jdemeyer/t22796 to 0ac7893146826afa62ffe437868e8c5d6d313c17
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.