Skip to content

Document extra dependencies#325

Merged
aaronayres35 merged 9 commits into
masterfrom
doc-extra-dependencies
Oct 21, 2020
Merged

Document extra dependencies#325
aaronayres35 merged 9 commits into
masterfrom
doc-extra-dependencies

Conversation

@aaronayres35
Copy link
Copy Markdown
Contributor

fixes #126

This PR adds extras_require to setup.py and also updates the readme to mention the optional dependencies.

The issue originally mentioned tornado but I am not sure this is still really needed? tornado is listed as a required dependency of ipykernel and the only occurrences of "tornado" in envisage are:

def configure_tornado_logger(self):
"""
Configure tornado logging.
Adds a NullHandler to the tornado root logger, if there are
no handlers already present on that logger. If no tornado
handler is present at the time the IO loop is started, tornado
will call logging.basicConfig, which isn't what we want to happen.
Overridden from the base class, which unconditionally adds a new
StreamHandler every time.
See also:
- https://github.com/tornadoweb/tornado/blob/v6.0.3/tornado/ioloop.py#L427-L445 # noqa: E501
- https://github.com/tornadoweb/tornado/pull/741
"""
logger = logging.getLogger("tornado")
if not logger.handlers:
logger.addHandler(logging.NullHandler())

if ipykernel_available:
import ipykernel.iostream
import ipykernel.ipkernel
import ipykernel.kernelapp
import ipykernel.zmqshell
import IPython.utils.io
import tornado.ioloop

and
def test_init_ipkernel_with_explicit_gui_backend(self):
loop = tornado.ioloop.IOLoop.current()

It also may be worth adding more details to the readme about what the optional dependencies are useful for / that the required dependencies are needed for the core envisage package. I was not sure how much to say here, and other enthought package readmes seem to be pretty brief.

Comment thread README.rst Outdated

To build the full documentation one needs:

* `Sphinx <https://pypi.org/project/Sphinx>`_ version 1.8 or later.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not know for sure if version >=1.8 is specifically required. Admittedly, I blindly took this from traits.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Traits needs Sphinx >= 2.1; looks like the README for Traits is out of date there. I think the restriction on the Sphinx version for Traits resulted from the needs of the trait-documenter and the trait-documenter tests. It's probably safe to use an earlier version here, but 2.1 is a good minimum version.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created enthought/traits#1320 to fix the Traits README

@aaronayres35
Copy link
Copy Markdown
Contributor Author

ref: Note this PR is also related to #154 as it updates part of the readme.

Comment thread setup.py
]
},
install_requires=["apptools", "setuptools", "traits"],
extras_require={
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I maybe should add "docs": ["enthought-sphinx-theme", "Sphinx!=3.2.0"] here as well. Additionally the keys here could be more specific.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tthat sounds like a good idea to me. The keys don't need to be anymore specific that what they are in traits.

Comment thread setup.py Outdated
Comment thread setup.py
]
},
install_requires=["apptools", "setuptools", "traits"],
extras_require={
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tthat sounds like a good idea to me. The keys don't need to be anymore specific that what they are in traits.

@mdickinson
Copy link
Copy Markdown
Member

The issue originally mentioned tornado but I am not sure this is still really needed?

For what it's worth, my usual rule with dependencies is that if something is explicitly imported from, it should be listed under the dependencies (even if it's a dependency of something else). So e.g. if a package imports from both traits and traitsui, the dependencies should list both, even though traits is a dependency of traitsui.

Co-authored-by: Poruri Sai Rahul <rporuri@enthought.com>
Comment thread README.rst Outdated

Envisage has the following optional dependencies:

* `Ipykernel <https://github.com/ipython/ipykernel>`_
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I link the github repo or the pypi page? Traits refers to pypi, I think pyface refers to the traits github. In gerenal maybe pypi links would make more sense? At the very least for non Enthought related dependencies like ipykernel

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed that PyPI links make most sense. People reading the README on GitHub probably don't really care; people reading the project description on PyPI are more likely to want PyPI links.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went with Pypi links for non enthought stuff, and github for enthought

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah I hadn't refreshed and just saw this comment, I will use pypi links for pyface and traitsui as well

…using pypi links on readme instead of github links
Copy link
Copy Markdown
Contributor

@rahulporuri rahulporuri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

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 this pull request may close these issues.

Document extra dependencies

3 participants