Rewrite deprecation to use trac ticket numbers

[vbraun]

As discussed on https://groups.google.com/d/topic/sage-devel/I12IeaFlE7g/discussion, change the deprecation function to the new arguments

deprecation(trac_number, message) 

where both arguments are mandatory. Once this code is in Sage, one
can deduce every possible thing discussed above in this thread from
the trac number. The deprecation warning can produce the URL of the
trac ticket.

Analogous changes are made to deprecated_function_alias and deprecated_callable_import. Finally, the @rename_keyword(deprecated="sage version string", ...) decorator is changed to

@rename_keyword(deprecation=<trac_number>, ...)

Apply

• attachment: trac_13109_deprecation.patch
• attachment: trac_13109_ticket_numbers.patch
• attachment: trac_13109_fix_doctests.patch
• attachment: trac_13109-documentation.v3.patch

This ticket also fixes #8073, #8546.

Depends on #12544
Depends on #12965

CC: @JohnCremona @kini @williamstein @jasongrout @kcrisman

Component: doctest coverage

Author: Volker Braun

Reviewer: John Palmieri, Karl-Dieter Crisman

Merged: sage-5.2.rc0

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

[kcrisman]

comment:1

I just have to say it, please don't hurt me!

Definition:     sage.misc.misc.deprecation(message, version=None)
Docstring:
       Issue a deprecation warning.
    
       INPUT:
    
          * "message" - an explanation why things are deprecated and by
            what it
               should be replaced.
    
          * "version" - (optional) on which version and when the
            deprecation
               occurred. Please put there the version of sage at the time
               of deprecation.

That's incompatible with the current one, as far as I understand the ways optional args with defaults work. So will the syntax for the deprecation function have to be ... deprecated?

[vbraun]

comment:2

That depends on whether the barber of Seville shaves himself or not.

[vbraun]

comment:3

The first patch changes the deprecation syntax. The second adds the trac numbers to all deprecations in the Sage library. The third fixes all doctests.

[vbraun]

Author: Volker Braun

[vbraun]

comment:5

The last patch adds documentation to the developer guide.

[kcrisman]

comment:7

Very comprehensive work.

• I first have to say that attachment: trac_13109_ticket_numbers.patch is extremely impressive. I guess that relieves any concerns about deprecating the deprecation functionality.
• Do we want to use the :trac: markup anywhere here? Of course, that doesn't show up as nicely in the command line, so perhaps not.
• So will it be up the end user to determine whether a given item is nearing the end of its deprecation life? I do like that part of add section on deprecating functions to developer's guide #8546. It could be really tedious to check whether a number of things are due. Why not have that as another (optional) argument? Maybe there is a good reason I'm just missing, certainly this syntax is easier for the person writing the code.
• Also, what happens if Trac goes the way of ... Mercurial for sagenb?
• I notice that Foo also has terrible doctesting, in addition to having several ill-conceived methods. But in general I really like it - very clear but also not dry.
• That said, there should be an example of how to doctest the deprecation, because it won't look like what comes out, but rather

doctest:...: DeprecationWarning: 
]}}
  as you know from the presumably tedious work on the doctest patch.

[kcrisman]

comment:8

Oops, removing that was a mistake.

One more thing; to make sure that this supersedes #8073, if we do keep Sage versions in, let's make sure to make it very very clear whether we are doing >= or >.

Also, reading through the ticket number patch confirms my sense that Sage version is important; looking at something that says Sage version 4.3 (!!!) is far more revealing than Trac number so and so.

[vbraun]

comment:9

The :trac: markup shows nicely in the html reference. I've opened #13116 to properly render it on the commandline.

If you record the trac number then its == and there is no question about >= or >.

We should collect deprecated ticket numbers during doctest so we can list them. Then its easy to list the deprecations sorted by age, say. But this is for another ticket.

[vbraun]

comment:10

I updated the patches to use the shorter url http://trac.sagemath.org/<number>, no other changes.

There is nothing different in doctesting deprecations from doctesting other stuff in the Sage library, so I'm against adding an extra section on that to the coding conventions section. Shorter is better if there is no additional information conveyed.

Now would be a good time to review this ticket. It touches a lot of places and I do have better things to do than to keep the patches up-to-date for the next 6 months :-P

[kcrisman]

comment:11

Replying to @vbraun:

I updated the patches to use the shorter url http://trac.sagemath.org/<number>, no other changes.

There is nothing different in doctesting deprecations from doctesting other stuff in the Sage library, so I'm against adding an extra section on that to the coding conventions section. Shorter is better if there is no additional information conveyed.

Simply not true. You have

doctest:1: DeprecationWarning: 

but one needs an ellipsis, and this is standard in the tests I've seen. Probably not always necessary, but good protocol.

doctest:...: DeprecationWarning: 

In fact, you even change this in several of the tests in your patch.

Also, I'm wondering why the tests pass in developer/conventions.rst when you don't use the doctest: at all. Is that itself completely optional? Now I'm confused.

In fact, even putting in a blatant error in that file doesn't cause a problem in testing. Do these even get tested?

[kcrisman]

comment:12

Oh, and as to reviewing this monster - I appreciate what you're saying, but actually checking that all these Trac numbers are correct is a big job all by itself. I'm sorry that I may not be able to get to this one soon.

[JohnCremona]

comment:13

Although I started this with my post to sage-devel, I'm afraid that I don't have time to test it as I am rushing for a deadline and will be away for 3 weeks with little opportunity.

Regarding checking all the trac numbers being correct: I'm sure that almost all, if not all are correct, since I am sure that Volker will have done a careful job. Hence I think that a positive review on this should not wait for every one to be checked. If any are wrong, as soon as anyone notices it can be fixed in a separate follow-up. Is that not sensible? Certianly better than having to do it all again in a few months!

[vbraun]

comment:14

We should write "Perfect is the enemy of good" in huge letters on top of the developer guide, I think.

[vbraun]

Removed trailing whitespace

[vbraun]

comment:15

Attachment: trac_13109_documentation.patch.gz

Hmm now the patchbot gets the order wrong.

Apply trac_13109_deprecation.patch, trac_13109_fix_doctests.patch, trac_13109_documentation.patch, trac_13109_ticket_numbers.patch

[vbraun]

comment:16

Me, too %-)

Apply trac_13109_deprecation.patch, trac_13109_ticket_numbers.patch, trac_13109_fix_doctests.patch, trac_13109_documentation.patch

[kwankyu]

comment:17

I am just curious why we don't use a consistent style in deprecating functions. I would prefer the following style using decorators as I like the @rename_keyword decorator. But I am not competent enough to be sure if it is technically sound or even possible to use decorators for other deprecation situations.

from sage.misc.decorators import rename_keyword, rename_function, deprecate 

class Foo(SageObject): 
	 
    @deprecate(3333,'You can just call f() instead') 
    def terrible_idea(self): 
        return 1 
 	 
    @rename_function(4444, bad_name)
    def much_better_name(self): 
        return 1 
  
    @rename_keyword(deprecation=5555, weird_keyword='nice_keyword') 
    def f(self, nice_keyword=True): 
        return 1

[vbraun]

comment:18

You can also use the `@rename_keyword' decorator to give alternative spellings for keywords (e.g. British vs. American English). Its not just a tool to deprecate keyword options.

[vbraun]

Dependencies: #12544

[jdemeyer]

comment:33

This needs to be rebased to sage-5.2.beta1:

sage -t  -force_lib devel/sage/sage/graphs/generic_graph.py
**********************************************************************
File "/release/merger/sage-5.2.rc0/devel/sage-main/sage/graphs/generic_graph.py", line 10906:
    sage: (graphs.FruchtGraph()).clustering_coeff(weight=True,
        return_vertex_weights=True)
Exception raised:
    Traceback (most recent call last):
      File "/release/merger/sage-5.2.rc0/local/bin/ncadoctest.py", line 1231, in run_one_test
        self.run_one_example(test, example, filename, compileflags)
      File "/release/merger/sage-5.2.rc0/local/bin/sagedoctest.py", line 38, in run_one_example
        OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
      File "/release/merger/sage-5.2.rc0/local/bin/ncadoctest.py", line 1172, in run_one_example
        compileflags, 1) in test.globs
      File "<doctest __main__.example_145[7]>", line 2, in <module>
        return_vertex_weights=True)
      File "/release/merger/sage-5.2.rc0/local/lib/python/site-packages/sage/graphs/generic_graph.py", line 10933, in clustering_coeff
        from sage.misc.misc import deprecation
    ImportError: cannot import name deprecation
**********************************************************************

[vbraun]

Updated patch

[vbraun]

Attachment: trac_13109_ticket_numbers.patch.gz

Updated patch

[vbraun]

comment:34

Attachment: trac_13109_fix_doctests.patch.gz

I've updated the patches to (the as of now unreleased) sage-5.2.beta1. I'm switching this back to positive review since it is a trivial rebase.

[nthiery]

comment:35

Hi guys!

I love this new syntax; it's a great way to point to the right piece of information! I also appreciate the tremendous work you put into recovering the ticket number for all deprecation warnings in Sage.

However, I am deeply concerned by the fact that this patch touches so many files in Sage. It is going to create conflicts with many upcoming patches, including many in the Sage-Combinat queue. We are already having great trouble getting things in and this would waste a lot of good developpers time.

I thus would strongly recommend, if not request, that this change be made with a transition period where including a track ticket number is mandatory for getting new/refactored code into Sage, but where old deprecation warnings could be left as is. Anyway, those deprecation warnings are doomed to progressively disapear by themselves; is it really worth refactoring them?

John: sorry for voicing this, ... well ... loudly, earlier today. Sorry also for not sumbling earlier on this ticket to give my view point before all the work is done.

Cheers,
Nicolas

[vbraun]

comment:36

Hi Nicolas,

How many conflicts do you actually get? The assumption is that deprecated code isn't under current development, so although we touch a lot of files we don't actually get many conflicts.

Many deprecation warnings didn't even have a "since Sage version x" attached, so its doubtful that they will be removed over time.

We can add a stub old-style sage.misc.misc.deprecation, but I think that'll just delay the pain of updating the deprecations until later since nobody will be forced to switch.

Best,
Volker

[jhpalmieri]

comment:37

A grep on the combinat queue tells me that 34 files use deprecation. Some of those don't need to be changed at all, but I would bet that at least 20 of them do.

[jdemeyer]

comment:38

Should we have

DeprecationWarning: You are using a deprecated way to signal deprecations.

:-)

[JohnCremona]

comment:39

Replying to @jdemeyer:

Should we have

DeprecationWarning: You are using a deprecated way to signal deprecations.

:-)

See comments 1 and 2 above (and I thought that was my joke!)

[jhpalmieri]

comment:40

I propose that we deprecate all self-referential deprecation jokes.

[kcrisman]

comment:41

I propose that we deprecate all self-referential deprecation jokes.

Never!

[jdemeyer]

Merged: sage-5.2.rc0