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.

Sign up for GitHub

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

Jump to bottom

Convert colorama into an optional dependency #40

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Convert colorama into an optional dependency #40

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 5 additions & 14 deletions icecream/icecream.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,16 +13,13 @@

from __future__ import print_function

import ast
import inspect
import pprint
import sys
from contextlib import contextmanager
from datetime import datetime
from os.path import basename
from textwrap import dedent

import colorama
import executing
from pygments import highlight
# See https://gist.github.com/XVilka/8346728 for color support in various
Expand All @@ -33,6 +30,10 @@

from .coloring import SolarizedDark

if sys.platform.startswith('win'):
# We will import colorama only on a specific subset of OS's thus removing colorama from required dependencies
import colorama
colorama.init()

PYTHON2 = (sys.version_info[0] == 2)

Expand All @@ -55,23 +56,13 @@ def colorize(s):
return highlight(s, self.lexer, self.formatter)


@contextmanager
def supportTerminalColorsInWindows():
# Filter and replace ANSI escape sequences on Windows with equivalent Win32
# API calls. This code does nothing on non-Windows systems.
colorama.init()
yield
colorama.deinit()
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could easily write this to still use a context manager and deinit after each print. Any reason not to?

Copy link
Author

@VaultVulp VaultVulp May 6, 2020 •
edited

Choose a reason for hiding this comment

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

Thanks for you interest to my PR, @alexmojaki .

I spent a little time tweaking with the code base, trying to minimize code change, but I wasn't able to find a readable solution.

If we leave this contex manager in place, we should move conditional import into this context manager, thus we will try to import colorama on each call of ic. And import statement is a costly opertaion.

I've tried to move context manager into another module (windows_support), to be able to import colorama on a module level. But in that case we have to use conditional function definition such as:

if sys.platform == 'win32':
    from .windows_support import supportTerminalColorsInWindows

    def colorizedStderrPrint(s):
        colored = colorize(s)
        with supportTerminalColorsInWindows():
            stderrPrint(colored)
else:
    def colorizedStderrPrint(s):
        colored = colorize(s)
        stderrPrint(colored)

In my opinion it comlicates both icecream.py and entrire package. So I had to throw away this idea.

After that I've read a little bit of colorama's documentation to find out the reason to deinit colorama, but wasn't succesfful. So I wasn't sure why do we need to call init and deinit in such manner.

After a while, I've rewriten my proposition from ground up, and created this PR.

If you can help me understand the reason behind this context manager, I will try to rewrite this PR again.

Copy link
Collaborator

Choose a reason for hiding this comment

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

See if you can do without init() and deinit() entirely using colorama.AnsiToWin32.

Copy link
Author

@VaultVulp VaultVulp May 6, 2020 •
edited

Choose a reason for hiding this comment

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

I guess we can write:

if sys.platform.startswith('win'):
    # We will import colorama only on a specific subset of OS's, and wrap sys.stderr into colorama's stream wrapper
    import colorama
    colorama.init(wrap=False)
    output_stream = colorama.AnsiToWin32(sys.stderr).stream
else:
    output_stream = sys.stderr

...

def stderrPrint(*args):
    print(*args, file=output_stream)

Sadly, in this case we have to rewrite captureStandardStreams context manager in test_icecream.py, for it tries to replace stderr with a fake stream. I tride to monkey-patch newly added module level attribute, but without any luck.



def stderrPrint(*args):
print(*args, file=sys.stderr)


def colorizedStderrPrint(s):
colored = colorize(s)
with supportTerminalColorsInWindows():
stderrPrint(colored)
stderrPrint(colored)


DEFAULT_PREFIX = 'ic| '
Expand Down
2 changes: 1 addition & 1 deletion setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ def run_tests(self):
],
tests_require=[],
install_requires=[
'colorama>=0.3.9',
'colorama>=0.3.9; platform_system=="Windows"',
'pygments>=2.2.0',
'executing>=0.3.1',
'asttokens>=2.0.1',
Expand Down