Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove deprecated_callable_import #22796

Closed
jdemeyer opened this issue Apr 11, 2017 · 28 comments
Closed

Remove deprecated_callable_import #22796

jdemeyer opened this issue Apr 11, 2017 · 28 comments

Comments

@jdemeyer
Copy link

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.

CC: @videlec

Component: misc

Author: Jeroen Demeyer

Branch/Commit: 0ac7893

Reviewer: Dima Pasechnik

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

@jdemeyer jdemeyer added this to the sage-8.0 milestone Apr 11, 2017
@jdemeyer

This comment has been minimized.

@jdemeyer

This comment has been minimized.

@jdemeyer jdemeyer changed the title Deprecate deprecated_callable_import Remove deprecated_callable_import Apr 11, 2017
@jdemeyer
Copy link
Author

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 11, 2017

Commit: 4b90c16

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 11, 2017

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

4b90c16Remove deprecated_callable_import

@videlec
Copy link
Contributor

videlec commented Apr 11, 2017

comment:6

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

@jdemeyer
Copy link
Author

comment:7

Replying to @videlec:

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).

@dimpase
Copy link
Member

dimpase commented May 9, 2017

Reviewer: Dima Pasechnik

@dimpase
Copy link
Member

dimpase commented May 9, 2017

comment:8

This looks good to me.

@vbraun
Copy link
Member

vbraun commented May 11, 2017

comment:9

Merge conflict...

@dimpase
Copy link
Member

dimpase commented May 11, 2017

comment:10

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

@vbraun
Copy link
Member

vbraun commented May 13, 2017

comment:12

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

@dimpase
Copy link
Member

dimpase commented May 13, 2017

Changed commit from 4b90c16 to 18e9a46

@dimpase
Copy link
Member

dimpase commented May 13, 2017

comment:13

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.

@dimpase
Copy link
Member

dimpase commented May 13, 2017

@vbraun
Copy link
Member

vbraun commented May 14, 2017

comment:14
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

@dimpase
Copy link
Member

dimpase commented May 14, 2017

comment:15

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)

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 14, 2017

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 14, 2017

Changed commit from 18e9a46 to e258a20

@dimpase
Copy link
Member

dimpase commented May 14, 2017

comment:17

oops, wrong branch

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 14, 2017

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

f92d174remove obsolete test

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 14, 2017

Changed commit from e258a20 to f92d174

@dimpase
Copy link
Member

dimpase commented May 14, 2017

comment:19

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

@jdemeyer
Copy link
Author

comment:20

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.

@jdemeyer
Copy link
Author

Changed branch from u/dimpase/t22796 to u/jdemeyer/t22796

@dimpase
Copy link
Member

dimpase commented May 15, 2017

Changed commit from f92d174 to 0ac7893

@dimpase
Copy link
Member

dimpase commented May 15, 2017

comment:22

Sage imports drive me mad elsewhere too: #22799 comment:90
:-)


New commits:

0ac7893Remove absolute_import from catalog

@vbraun
Copy link
Member

vbraun commented May 16, 2017

Changed branch from u/jdemeyer/t22796 to 0ac7893

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants