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

Too many functions from databases/cremona.py are exported globally #10107

Closed
JohnCremona opened this issue Oct 9, 2010 · 18 comments
Closed

Too many functions from databases/cremona.py are exported globally #10107

JohnCremona opened this issue Oct 9, 2010 · 18 comments

Comments

@JohnCremona
Copy link
Member

In sage/databases/all.py:

from cremona import CremonaDatabase, \
     cremona_letter_code, parse_cremona_label, \
     old_cremona_letter_code, is_optimal_id

but only the first of these should be global.

Component: elliptic curves

Keywords: databases cremona

Author: Mike Hansen

Reviewer: Robert Miller, Aly Deines

Merged: sage-4.6.1.alpha2

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

@JohnCremona JohnCremona added this to the sage-4.6.1 milestone Oct 9, 2010
@JohnCremona JohnCremona self-assigned this Oct 9, 2010
@mwhansen
Copy link
Contributor

mwhansen commented Oct 9, 2010

Attachment: trac_10107.patch.gz

@mwhansen
Copy link
Contributor

Author: Mike Hansen

@mwhansen
Copy link
Contributor

comment:1

This needs to be coordinated with #9907.

@JohnCremona
Copy link
Member Author

comment:2

I don't really understand the connection between this and #9907, or what most of the patch is for! Is it just that you don't want to get rid of a command which used to work from a sage: prompt without deprecating it first?

@mwhansen
Copy link
Contributor

comment:3

This patch has from sage.misc.misc import sage_wraps while the patch at #9907 moves sage_wraps from sage.misc.misc to sage.misc.decorators.

Yes, most of the patch is to deprecate using those functions from the top level before removing them. Since this shared some functionality with preexisting code, the idea was to unify the code.

@rlmill
Copy link
Mannequin

rlmill mannequin commented Nov 9, 2010

comment:4

If I apply #6094, #9907, #9919, and #10107 together on top of sage-4.6, all long tests pass. The code looks good.

@rlmill
Copy link
Mannequin

rlmill mannequin commented Nov 9, 2010

Reviewer: Robert Miller

@rlmill rlmill mannequin added s: positive review and removed s: needs review labels Nov 9, 2010
@jdemeyer
Copy link
Contributor

comment:5

I get various doctest errors in sage/databases/cremona.py:

File "/mnt/usb1/scratch/jdemeyer/merger/sage-4.6.1.alpha2/devel/sage-main/sage/databases/cremona.py", line 102:
    sage: is_optimal_id('b1')
Expected:
    True
Got:
    doctest:1: DeprecationWarning:
    Using is_optimal_id from here is deprecated.  If you need to use it, please import it directly from sage.databases.cremona.
    True

and several more like this.

@jdemeyer
Copy link
Contributor

Changed keywords from databases to databases cremona

@JohnCremona
Copy link
Member Author

Attachment: trac_10107-fix.patch.gz

replaces previous

@JohnCremona
Copy link
Member Author

comment:6

The new patch deals with this (replaces previous).

@adeines
Copy link
Mannequin

adeines mannequin commented Nov 16, 2010

Changed reviewer from Robert Miller to Robert Miller, Alyson Deines

@jdemeyer
Copy link
Contributor

Merged: sage-4.6.1.alpha2

@jdemeyer
Copy link
Contributor

Changed reviewer from Robert Miller, Alyson Deines to Robert Miller, Aly Deines

@eviatarbach
Copy link
Contributor

comment:11

It's been over two years since these were deprecated; can they be removed from the global namespace?

@JohnCremona
Copy link
Member Author

comment:12

I am happy with that. I guess it needs a new ticket. Will you make one and reference it from here?

@eviatarbach
Copy link
Contributor

comment:13

Okay, good. The new ticket is #14642.

@jdemeyer
Copy link
Contributor

comment:14

Interestingly, this ticket is the one which introduced deprecated_callable_import. Given that I see no advantage of deprecated_callable_import over a lazy import with deprecation, I plan to remove deprecated_callable_import in #22796.

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