Opened 6 years ago

Closed 5 years ago

#15076 closed defect (fixed)

remove import of is_* in rings.all

Reported by: chapoton Owned by:
Priority: major Milestone: sage-5.13
Component: build Keywords: deprecation cleanup
Cc: ppurka, ncohen Merged in: sage-5.13.beta1
Authors: Frédéric Chapoton Reviewers: Nathann Cohen
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

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

Attachments (1)

trac_15076_remove_global_is_in_rings.patch (65.2 KB) - added by chapoton 6 years ago.
replaces previous patch

Download all attachments as: .zip

Change History (23)

comment:1 Changed 6 years ago by chapoton

  • Authors set to Frédéric Chapoton
  • Status changed from new to needs_review

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.

comment:2 Changed 6 years ago by chapoton

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.

comment:3 Changed 6 years ago by ppurka

  • Cc ppurka added

comment:4 Changed 6 years ago by chapoton

  • Status changed from needs_review to needs_work

I have made sure that tests pass in

  • schemes/elliptic_curves
  • combinat

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

comment:5 Changed 6 years ago by chapoton

  • Status changed from needs_work to needs_review

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

comment:6 Changed 6 years ago by chapoton

  • Cc ncohen added

comment:7 Changed 6 years ago by chapoton

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

comment:8 Changed 6 years ago by chapoton

apply only trac_15076_remove_global_is_in_rings.patch​

comment:9 Changed 6 years ago by chapoton

apply only trac_15076_remove_global_is_in_rings.patch

comment:10 Changed 6 years ago by chapoton

apply only trac_15076_remove_global_is_in_rings.patch

Changed 6 years ago by chapoton

replaces previous patch

comment:11 Changed 6 years ago by chapoton

apply only trac_15076_remove_global_is_in_rings.patch

comment:12 Changed 6 years ago by chapoton

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

comment:13 Changed 6 years ago by ncohen

  • Status changed from needs_review to positive_review

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

Nathann

comment:14 Changed 6 years ago by ncohen

  • Status changed from positive_review to needs_info

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

comment:15 Changed 6 years ago by ppurka

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

comment:16 Changed 6 years ago by ncohen

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

comment:17 Changed 6 years ago by chapoton

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.

comment:18 Changed 6 years ago by ncohen

  • Reviewers set to Nathann Cohen
  • Status changed from needs_info to positive_review

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

comment:19 Changed 6 years ago by ncohen

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

comment:20 Changed 6 years ago by ppurka

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

comment:21 Changed 5 years ago by jdemeyer

  • Milestone changed from sage-5.12 to sage-5.13

comment:22 Changed 5 years ago by jdemeyer

  • Merged in set to sage-5.13.beta1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.