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

Consider making pygments an optional dependency for pyface.api #655

Closed
kitchoi opened this issue Aug 4, 2020 · 6 comments · Fixed by #796
Closed

Consider making pygments an optional dependency for pyface.api #655

kitchoi opened this issue Aug 4, 2020 · 6 comments · Fixed by #796

Comments

@kitchoi
Copy link
Contributor

kitchoi commented Aug 4, 2020

pygments is only required for CodeEditor and PythonShell (qt). For use cases where neither of these objects are required, one still needs to have pygments in the environment in order to import anything from pyface.api.

Consider deferring this dependency until the said widgets are required.

@mdickinson
Copy link
Member

Related: #446

@corranwebster
Copy link
Contributor

Confusing terminology here: Pygments is an optional dependency currently:

"pyqt": ["pyqt>=4.10", "pygments"],
"pyqt5": ["pyqt5", "pygments"],
"pyside": ["pyside>=1.2", "pygments"],
"pyside2": ["pyside2", "shiboken2", "pygments"],

I think what you are suggesting is shifting the imports out of top-level modules and into the functions/methods where they are used?

@kitchoi
Copy link
Contributor Author

kitchoi commented Nov 9, 2020

Hmmm, I am thinking of maybe something like this:

__extras_require__ = {
    "wx": ["wxpython>=4", "numpy"],
    "pyqt": ["pyqt>=4.10"],
    "pyqt5": ["pyqt5"],
    "pyside": ["pyside>=1.2"],
    "pyside2": ["pyside2", "shiboken2"],
    "test": ["packaging"],
    "all_features": ["pygments"],    # <---- add this for dependencies on all features.
}

@corranwebster
Copy link
Contributor

No, I think the dependencies are best kept as-is. Pygments isn't a huge dependency and a lot of the examples use the code widget, and having it as an "all_features" implies that it is used by wx (which it isn't).

I would be OK with shifting imports so that it is imported at use time (if possible - it may not be), so if you don't have it installed you don't get an immediate error. But I'd also be OK with leaving it as-is to avoid the support issues of the form "the code editor is broken".

The issue which graphcanvas is experiencing is really due to a limitation in EDM, and this doesn't help with that in any case.

@kitchoi
Copy link
Contributor Author

kitchoi commented Nov 10, 2020

Just to clarify... I think when I included the code snipte for the __extras_require__, I also have in mind to modify pyface.api similar to how traitsui.api treats ArrayEditor:
https://github.com/enthought/traitsui/blob/581360c2cb2a040bb5309a9c61d84361a849be10/traitsui/api.py#L29-L33

Sorry for omitting to mention that too (I thought the original comment implied that already).

@kitchoi
Copy link
Contributor Author

kitchoi commented Nov 10, 2020

Actually, that might be a better way to implement this... The goal is such that if one has Qt in the environment, importing pyface.api does not have to fail just because one does not have pygments in their environment. (This has annoyed me far too many times.)

If someone needs to import PythonEditor and they did not have pygments, then they should install it (it is understandable to require pygments for a feature that provides highlighting capability). In any case, these optional dependencies should be documented, just like wxPython/PySide2/... being optional dependencies.

(The "all_features" name can be changed to be more specific, but it is roughly the same idea.)

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

Successfully merging a pull request may close this issue.

3 participants