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 import of is_* in rings.all #15076

Closed
fchapoton opened this issue Aug 22, 2013 · 22 comments
Closed

remove import of is_* in rings.all #15076

fchapoton opened this issue Aug 22, 2013 · 22 comments

Comments

@fchapoton
Copy link
Contributor

As a big first step towards #14329, I propose to remove all imports of methods is_[A-Z] in sage/rings/all.py

CC: @ppurka @nathanncohen

Component: build

Keywords: deprecation cleanup

Author: Frédéric Chapoton

Reviewer: Nathann Cohen

Merged: sage-5.13.beta1

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

@fchapoton fchapoton added this to the sage-5.12 milestone Aug 22, 2013
@fchapoton
Copy link
Contributor Author

Author: Frédéric Chapoton

@fchapoton
Copy link
Contributor Author

comment:1

This should be ok, unless I have made a mistake

This is a little "patchbomb", so it would be great if it could be reviewed quickly.

@fchapoton
Copy link
Contributor Author

comment:2

Well, I have only taken care of what happens at startup !

There probably remains many imports not triggered at startup..

Let us see what the bot says.

@fchapoton
Copy link
Contributor Author

comment:4

I have made sure that tests pass in

  • schemes/elliptic_curves
  • combinat

but of course, there are many other places where to look ..

@fchapoton
Copy link
Contributor Author

comment:5

ok, this should be ready for review. It would be great if the bot could run on the ticket..

apply trac_15076_remove_global_is_in_rings.patch

@fchapoton
Copy link
Contributor Author

comment:7

new version of the patch, that hopefully the bot will like

@fchapoton
Copy link
Contributor Author

comment:8

apply only trac_15076_remove_global_is_in_rings.patch​

@fchapoton
Copy link
Contributor Author

comment:9

apply only trac_15076_remove_global_is_in_rings.patch

@fchapoton
Copy link
Contributor Author

comment:10

apply only trac_15076_remove_global_is_in_rings.patch

@fchapoton
Copy link
Contributor Author

Attachment: trac_15076_remove_global_is_in_rings.patch.gz

replaces previous patch

@fchapoton
Copy link
Contributor Author

comment:11

apply only trac_15076_remove_global_is_in_rings.patch

@fchapoton
Copy link
Contributor Author

comment:12

Hey, the bot is happy ! Is there anybody wanting to review this patch ?

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Sep 3, 2013

comment:13

Oh. Well, if it is happy... :-)

Nathann

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Sep 3, 2013

comment:14

Hmmmmmmmm.... O_o

Actually I wonder if those methods shouldn't be deprecated first, as those who used them may not know where they moved... O_o

I used deprecated_callable_import in #14499, isn't it what should be used there too ?

Nathann

@ppurka
Copy link
Member

ppurka commented Sep 3, 2013

comment:15

These were deprecated for many years. Try them in vanilla sage.

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Sep 3, 2013

comment:16

Oh I see ! But then why don't we just remove the deprecated version of the functions too ? It looks like the deprecated functions are not in the namespace anymore, but that they are still defined -- and deprecated somewhere O_o ?

Nathann

@fchapoton
Copy link
Contributor Author

comment:17

What do you mean ?

These functions are not deprecated, it's their import in the global namespace that is.

By the way, even after these patch, some of them will still be imported in the global namespace, and a follow-up ticket will be needed. I prefer to proceed step-by-step.

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Sep 4, 2013

comment:18

Oh. Well, what I wondered is which part of the code deprecated the version of these is_* functions that appears in the global namespace. I wondered why this code would not be removed by the current patch.

But I just noticed that it was actually a global piece of code, which deprecated all these is_* functions together, and thus is not to be removed by this patch.

Anyway : all is good, and this patch can go. My excuses for the delay ;-)

Nathann

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Sep 4, 2013

Reviewer: Nathann Cohen

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Sep 4, 2013

comment:19

(and here is where this global piece of code was added, just in case somebody else would like to know : #10107)

@ppurka
Copy link
Member

ppurka commented Sep 4, 2013

comment:20

Thanks for looking into this Nathann. I really didn't get time to dig into this patch.

@jdemeyer jdemeyer modified the milestones: sage-5.12, sage-5.13 Oct 1, 2013
@jdemeyer
Copy link
Contributor

Merged: sage-5.13.beta1

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

3 participants