Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix global import of sage.repl.rich_output.display_manager.get_display_manager #33016

Closed
mkoeppe opened this issue Dec 13, 2021 · 16 comments
Closed

Comments

@mkoeppe
Copy link
Member

mkoeppe commented Dec 13, 2021

This function leaks in via sage.interfaces.all.

Since it is being used in doctest, we import it properly through sage.repl.all.

CC: @egourgoulhon

Component: user interface

Author: Matthias Koeppe

Branch/Commit: d6cf9f6

Reviewer: Kwankyu Lee

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

@mkoeppe mkoeppe added this to the sage-9.5 milestone Dec 13, 2021
@mkoeppe
Copy link
Member Author

mkoeppe commented Dec 13, 2021

@mkoeppe
Copy link
Member Author

mkoeppe commented Dec 13, 2021

Commit: 5c06046

@mkoeppe
Copy link
Member Author

mkoeppe commented Dec 13, 2021

New commits:

5c06046src/sage/interfaces/all.py: Deprecate re-export of get_display_manager

@mkoeppe
Copy link
Member Author

mkoeppe commented Dec 13, 2021

Author: Matthias Koeppe

@kwankyu
Copy link
Collaborator

kwankyu commented Dec 14, 2021

comment:3

I have used this function only for testing. But it seems people use get_display_manager to set display preferences in jupyter. It would be inconvenient if it should be imported first from an obscure place.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 19, 2022

Changed commit from 5c06046 to d6cf9f6

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 19, 2022

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

738b703src/sage/repl/all.py: Also import get_display_manager
d6cf9f6src/sage/interfaces/all.py: Do not re-export get_display_manager; do not fail if get_display_manager cannot be imported

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Member Author

mkoeppe commented Feb 19, 2022

comment:6

Here's a new version

@mkoeppe mkoeppe changed the title Deprecate global import of sage.repl.rich_output.display_manager.get_display_manager Fix global import of sage.repl.rich_output.display_manager.get_display_manager Feb 19, 2022
@kwankyu
Copy link
Collaborator

kwankyu commented Feb 20, 2022

comment:7

Perhaps we still need a deprecation warning about the change of import location?

@kwankyu
Copy link
Collaborator

kwankyu commented Feb 20, 2022

Reviewer: Kwankyu Lee

@mkoeppe
Copy link
Member Author

mkoeppe commented Feb 20, 2022

comment:9

Replying to @kwankyu:

Perhaps we still need a deprecation warning about the change of import location?

I'd say we don't, according to the leak theory

@kwankyu
Copy link
Collaborator

kwankyu commented Feb 20, 2022

comment:10

Replying to @mkoeppe:

Replying to @kwankyu:

Perhaps we still need a deprecation warning about the change of import location?

I'd say we don't, according to the leak theory

Sorry, but what is the leak theory?

One argument for no deprecation warning might be that we already made an announcement that no library code should import from all modules. Is this what you meant?

@mkoeppe mkoeppe modified the milestones: sage-9.6, sage-9.7 Apr 11, 2022
@kwankyu
Copy link
Collaborator

kwankyu commented Apr 14, 2022

comment:12

Replying to @kwankyu:

Perhaps we still need a deprecation warning about the change of import location?

I still don't know the leak theory. But I now agree that we don't need a deprecation warning.

@mkoeppe
Copy link
Member Author

mkoeppe commented Apr 14, 2022

comment:13

Thanks!

@vbraun
Copy link
Member

vbraun commented May 24, 2022

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants