jupyter / nbdime

Tools for diffing and merging of Jupyter notebooks.
http://nbdime.readthedocs.io
Other
2.67k stars 160 forks source link

non-platform specific import of colorama breaks install from requirements.txt #602

Open itcarroll opened 3 years ago

itcarroll commented 3 years ago

prettyprint.py#L21 does not use the platforms module to restrict import to Windows. After pip installing nbdime from a requirements file that correctly causes pip to ignore colorama on a non-windows platform, the import of nbdime fails with ModuleNotFoundError exception:

Python 3.8.10 (default, Jun  2 2021, 10:49:15) 
[GCC 9.4.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import nbdime
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/lib/python3.8/dist-packages/nbdime/__init__.py", line 10, in <module>
    from .merging import merge_notebooks, decide_merge, apply_decisions
  File "/usr/local/lib/python3.8/dist-packages/nbdime/merging/__init__.py", line 6, in <module>
    from .generic import decide_merge
  File "/usr/local/lib/python3.8/dist-packages/nbdime/merging/generic.py", line 13, in <module>
    from .strategies import (
  File "/usr/local/lib/python3.8/dist-packages/nbdime/merging/strategies.py", line 20, in <module>
    from ..prettyprint import merge_render
  File "/usr/local/lib/python3.8/dist-packages/nbdime/prettyprint.py", line 21, in <module>
    import colorama
ModuleNotFoundError: No module named 'colorama'
>>> 

Installing colorama is a workaround until fixed.

vidartf commented 3 years ago

Good catch! Would you want to submit a PR for this? Or would you prefer if someone else did that?

itcarroll commented 3 years ago

Sorry, can't. I don't know what would replace the uses of colorama.Fore on L58-60.

It also looks like the solution should use from .utils import setup_std_streams, but that's getting into internals I wouldn't want to guess at.