Opened 9 years ago
Closed 8 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, GitHub, GitLab) | Commit: | a4848093884df99a12d031dc853ce9d97c0c3dc2 |
| Dependencies: | Stopgaps: |
Status badges
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)
Change History (14)
Changed 9 years ago by
comment:1 Changed 9 years ago by
- Status changed from new to needs_review
comment:2 Changed 9 years ago by
- Reviewers set to Travis Scrimshaw
- Status changed from needs_review to needs_work
- Work issues set to failing doctest
comment:3 Changed 9 years ago by
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.
comment:4 Changed 9 years ago by
- Milestone changed from sage-5.11 to sage-5.12
comment:5 Changed 8 years ago by
- Milestone changed from sage-6.1 to sage-6.2
comment:6 Changed 8 years ago by
- 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:
| 108e019 | Adds a deprecation option to lazy_import.
|
comment:7 Changed 8 years ago by
- Work issues failing doctest deleted
comment:8 Changed 8 years ago by
- Commit changed from 108e019185bf79a5e66c93a478f05116789278ae to a4848093884df99a12d031dc853ce9d97c0c3dc2
Branch pushed to git repo; I updated commit sha1. New commits:
| a484809 | Doctest fixes.
|
comment:9 Changed 8 years ago by
- 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 8 years ago by
- 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 8 years ago by
- 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 8 years ago by
Thanks Nathann.
comment:13 Changed 8 years ago by
- Branch changed from u/tscrim/14275 to a4848093884df99a12d031dc853ce9d97c0c3dc2
- Resolution set to fixed
- Status changed from positive_review to closed
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