Rewrite deprecation to use trac ticket numbers

[Volker Braun]

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

• trac_13109_deprecation.patch​
• trac_13109_ticket_numbers.patch​
• trac_13109_fix_doctests.patch​
• trac_13109-documentation.v3.patch​

This ticket also fixes #8073, #8546.

[Karl-Dieter Crisman]

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?

[Volker Braun]

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

[Volker Braun]

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.

[Volker Braun]

The last patch adds documentation to the developer guide.

[Karl-Dieter Crisman]

Very comprehensive work.

• I first have to say that 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 #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.
  

[Karl-Dieter Crisman]

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.

[Volker Braun]

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.

[Volker Braun]

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

[Karl-Dieter Crisman]

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?

[Karl-Dieter Crisman]

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.

[John Cremona]

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!

[Volker Braun]

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

[Volker Braun]

Removed trailing whitespace

[Volker Braun]

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

[Volker Braun]

Me, too %-)

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

[Kwankyu Lee]

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

[Volker Braun]

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.

[Volker Braun]

Updated patch

[Volker Braun]

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

[Volker Braun]

Patches updated for sage-5.1.rc0

[John Palmieri]

Regarding the documentation: it looks great, but I have three suggestions: first, change "user's" to "users'" in line 4 of the first paragraph. Second, make it a section instead of a subsection so it shows up in the main table of contents for the developer's guide (change the hyphens ----- to equals signs ======). Third, move it to the chapter on "coding in python": right now it's in the middle of the doctesting stuff, and the Python chapter contains information relevant to both Python and Cython (despite the chapter title -- maybe the chapter should be "Coding in Python and Cython" and the following one should be "Issues specific to coding in Cython", but anyway...). I'm attaching a version of the patch making these changes.

For the "ticket numbers" and "fix doctests" patches, I've done a bit of spot-checking, and it all looks good. The "deprecation" patch basically moves the deprecation code from misc.py to superseded.py, with some small modifications, right? That looks good, too. Is there any reason to do any error-checking on the trac number argument? Right now, using

def foo():
    sage.misc.superseded.deprecation('blah', 'the function foo is deprecated.')

works without error. (I'm fine with the current state of affairs, I'm just asking the question.)

Anyway, positive review for everything except for my version of the documentation.

[Volker Braun]

Updated patch

[Volker Braun]

change in deprecation.patch - for review purposes only

[Volker Braun]

Updated patch

[Volker Braun]

Updated patch

[Volker Braun]

Thanks for copy-editing the developer guide section, looks great. With your patch I got a doctest failure in coding_in_python.rst so I combined the final example into one doctestable example. I don't understand why this wasn't doctested before, but it passes now.

Since the deprecation is moved to its own module I didn't need any error checking - any old code failed with an import error. But for the future its probably good to check that the ticket number is a number. I've amended the trac_13109_deprecation.patch with error checking. The change is in trac_number_check.diff​

If you agree with these final changes then our work here will be finished ;-)

Patchbot: apply trac_13109_deprecation.patch, trac_13109_ticket_numbers.patch, trac_13109_fix_doctests.patch, trac_13109-documentation.v3.patch

[John Palmieri]

I agree with the changes, but I'm not sure the work is finished. I just looked at the currently positively-reviewed not-yet-merged tickets, and some of them may conflict, or have errors when deprecating functions, with the patches here: #2607, #9704, #11310 (applies with fuzz 2), #11143, #12768, #12840, #13100 (applies and passes tests, but includes an invalid use of deprecation).

I suggest rebasing each of those tickets on top of this one, rather than making this one depend on all of those -- if we did that, I worry that if just one of those had to be backed out, then this one would, also. I'll work on some and you can review my changes, and vice versa for the others. Sound okay? I'll start with the high numbers and work my way down.

[Volker Braun]

OK good.

[Andrzej Giniewicz]

Added dependencies - without those 5 tickets I wasn't able to apply patches to test #2607.

[Volker Braun]

aginiewicz: those are all in sage-5.1.rc0

[Jeroen Demeyer]

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
**********************************************************************

[Volker Braun]

Updated patch

[Volker Braun]

Updated patch

[Volker Braun]

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.

[Nicolas M. Thiéry]

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

[Volker Braun]

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

[John Palmieri]

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.

[Jeroen Demeyer]

Should we have

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

:-)

[John Cremona]

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

[John Palmieri]

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

[Karl-Dieter Crisman]

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

Never!