Opened 7 years ago

Closed 6 years ago

#14275 closed enhancement (fixed)

Lazy imports with deprecation

Reported by: roed Owned by: jason
Priority: major Milestone: sage-6.2
Component: misc Keywords:
Cc: Merged in:
Authors: David Roe, Travis Scrimshaw Reviewers: Travis Scrimshaw, Nathann Cohen
Report Upstream: N/A Work issues:
Branch: a484809 (Commits) Commit: a4848093884df99a12d031dc853ce9d97c0c3dc2
Dependencies: Stopgaps:

Description

It's annoying when other people move something around in the Sage library and it's no longer available in the previous location to be imported. Sage currently has some mechanisms for alleviating this problem (sage.misc.superceded.deprecated_callable_import and sage.structure.sage_object.register_unpickle_override for example). This ticket adds another option: lazily import the old name so that a deprecation warning is issued whenever it is referred to.

Attachments (1)

14275.patch (4.4 KB) - added by roed 7 years ago.

Download all attachments as: .zip

Change History (14)

Changed 7 years ago by roed

comment:1 Changed 7 years ago by roed

  • Status changed from new to needs_review

comment:2 Changed 7 years ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to needs_work
  • Work issues set to failing doctest

Hey David,

The doctest is failing because the mismatch of the trac numbers. Also could you add a doctest with a message as well?

Thanks,
Travis

comment:3 Changed 7 years ago by vbraun

The way I see it, deprecation only makes sense for stuff that is imported into the global namespace. Anything that is not directly accessible from the command line is, by definition, an implementation detail. And once you require backwards compatibility for implementation details it basically becomes impossible to change anything. So there is no point in deprecating import locations.

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

comment:4 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:5 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:6 Changed 6 years ago by tscrim

  • Branch set to u/tscrim/14275
  • Commit set to 108e019185bf79a5e66c93a478f05116789278ae
  • Status changed from needs_work to needs_review

This could be used to remove things from the global namespace but the class itself doesn't change.


New commits:

108e019Adds a deprecation option to lazy_import.

comment:7 Changed 6 years ago by tscrim

  • Work issues failing doctest deleted

comment:8 Changed 6 years ago by git

  • Commit changed from 108e019185bf79a5e66c93a478f05116789278ae to a4848093884df99a12d031dc853ce9d97c0c3dc2

Branch pushed to git repo; I updated commit sha1. New commits:

a484809Doctest fixes.

comment:9 Changed 6 years ago by ncohen

  • Status changed from needs_review to needs_info

Comment from the sage-combinat thread :

the way it is implemented you add a "deprecation" feature to lazy import. But lazy imports can be used a lot, so wouldn't it be better to instead add a "lazy" feature to "deprecation_function_alias" ?

Nathann

comment:10 Changed 6 years ago by tscrim

  • Status changed from needs_info to needs_review

No, because then we'd have to import deprecated_function_alias (which doesn't currently accept a message too) and import the original object just to do something like

x = deprecation_function_alias(14275, x)

Otherwise we completely change the syntax and functionality to include lazy_import. Both of which are asinine.

comment:11 Changed 6 years ago by ncohen

  • Authors changed from David Roe to David Roe, Travis Scrimshaw
  • Reviewers changed from Travis Scrimshaw to Travis Scrimshaw, Nathann Cohen
  • Status changed from needs_review to positive_review

Ahahaahha. Well, for as long as you know that the two reasons you gave me are bad reasons and can be easily avoided, all is fine. It is not so bad.

Good to go !

Nathann

comment:12 Changed 6 years ago by tscrim

Thanks Nathann.

comment:13 Changed 6 years ago by vbraun

  • Branch changed from u/tscrim/14275 to a4848093884df99a12d031dc853ce9d97c0c3dc2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.