Opened 8 years ago

Closed 7 years ago

#13109 closed enhancement (fixed)

Rewrite deprecation to use trac ticket numbers

Reported by: vbraun Owned by: mvngu
Priority: major Milestone: sage-5.2
Component: doctest coverage Keywords:
Cc: cremona, kini, was, jason, kcrisman Merged in: sage-5.2.rc0
Authors: Volker Braun Reviewers: John Palmieri, Karl-Dieter Crisman
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #12544, #12965 Stopgaps:

Description (last modified by 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

This ticket also fixes #8073, #8546.

Attachments (9)

trac_13109_documentation.patch (3.4 KB) - added by vbraun 8 years ago.
Removed trailing whitespace
trac_13109_ticket_numbers.2.patch (110.0 KB) - added by vbraun 7 years ago.
Updated patch
trac_13109-documentation.v2.patch (3.5 KB) - added by jhpalmieri 7 years ago.
trac_13109_deprecation.2.patch (28.5 KB) - added by vbraun 7 years ago.
Updated patch
trac_number_check.diff (2.5 KB) - added by vbraun 7 years ago.
change in deprecation.patch - for review purposes only
trac_13109_deprecation.patch (28.5 KB) - added by vbraun 7 years ago.
Updated patch
trac_13109-documentation.v3.patch (3.6 KB) - added by vbraun 7 years ago.
Updated patch
trac_13109_ticket_numbers.patch (110.8 KB) - added by vbraun 7 years ago.
Updated patch
trac_13109_fix_doctests.patch (74.1 KB) - added by vbraun 7 years ago.
Updated patch

Download all attachments as: .zip

Change History (51)

comment:1 Changed 8 years ago by kcrisman

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?

comment:2 Changed 8 years ago by vbraun

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

comment:3 Changed 8 years ago by vbraun

  • Description modified (diff)

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.

comment:4 Changed 8 years ago by vbraun

  • Description modified (diff)

comment:5 Changed 8 years ago by vbraun

  • Authors set to Volker Braun
  • Description modified (diff)

The last patch adds documentation to the developer guide.

comment:6 Changed 8 years ago by vbraun

  • Cc cremona kini was jason kcrisman added
  • Status changed from new to needs_review

comment:7 Changed 8 years ago by kcrisman

  • Description modified (diff)

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.
    

comment:8 Changed 8 years ago by kcrisman

  • Description modified (diff)

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.

Last edited 8 years ago by kcrisman (previous) (diff)

comment:9 Changed 8 years ago by vbraun

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.

comment:10 follow-up: Changed 8 years ago by 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.

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

comment:11 in reply to: ↑ 10 Changed 8 years ago by kcrisman

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?

comment:12 Changed 8 years ago by kcrisman

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.

comment:13 Changed 8 years ago by 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!

comment:14 Changed 8 years ago by vbraun

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

Changed 8 years ago by vbraun

Removed trailing whitespace

comment:15 Changed 8 years ago by vbraun

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

comment:16 Changed 8 years ago by vbraun

Me, too %-)

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

comment:17 Changed 7 years ago by klee

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
Last edited 7 years ago by klee (previous) (diff)

comment:18 Changed 7 years ago by vbraun

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.

comment:19 Changed 7 years ago by vbraun

  • Dependencies set to #12544

Changed 7 years ago by vbraun

Updated patch

comment:20 Changed 7 years ago by vbraun

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

comment:21 Changed 7 years ago by vbraun

Patches updated for sage-5.1.rc0

comment:22 Changed 7 years ago by jhpalmieri

  • Description modified (diff)
  • Reviewers set to John Palmieri, Karl-Dieter Crisman

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.

Changed 7 years ago by jhpalmieri

comment:23 Changed 7 years ago by jhpalmieri

  • Description modified (diff)

Changed 7 years ago by vbraun

Updated patch

Changed 7 years ago by vbraun

change in deprecation.patch - for review purposes only

Changed 7 years ago by vbraun

Updated patch

Changed 7 years ago by vbraun

Updated patch

comment:24 Changed 7 years ago by vbraun

  • Description modified (diff)

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

comment:25 Changed 7 years ago by jhpalmieri

  • Status changed from needs_review to positive_review

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.

Last edited 7 years ago by jhpalmieri (previous) (diff)

comment:26 Changed 7 years ago by vbraun

OK good.

comment:27 Changed 7 years ago by aginiewicz

  • Dependencies changed from #12544 to #12438, #12469, #12544, #12893, #13045, #13012

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

Last edited 7 years ago by aginiewicz (previous) (diff)

comment:28 Changed 7 years ago by vbraun

aginiewicz: those are all in sage-5.1.rc0

comment:29 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-5.1 to sage-5.2

comment:30 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-5.2 to sage-pending

comment:31 Changed 7 years ago by jdemeyer

  • Dependencies changed from #12438, #12469, #12544, #12893, #13045, #13012 to #12544, #12965

comment:32 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-pending to sage-5.2

comment:33 Changed 7 years ago by jdemeyer

  • Status changed from positive_review to needs_work

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

Changed 7 years ago by vbraun

Updated patch

Changed 7 years ago by vbraun

Updated patch

comment:34 Changed 7 years ago by vbraun

  • Status changed from needs_work to positive_review

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.

comment:35 Changed 7 years ago by nthiery

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

comment:36 Changed 7 years ago by vbraun

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

comment:37 Changed 7 years ago by jhpalmieri

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.

comment:38 follow-up: Changed 7 years ago by jdemeyer

Should we have

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

:-)

comment:39 in reply to: ↑ 38 Changed 7 years ago by 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!)

comment:40 follow-up: Changed 7 years ago by jhpalmieri

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

comment:41 in reply to: ↑ 40 Changed 7 years ago by kcrisman

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

Never!

comment:42 Changed 7 years ago by jdemeyer

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