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

Allow V2 console rules to use colors #7689

Merged
merged 4 commits into from May 9, 2019

Conversation

Projects
None yet
3 participants
@Eric-Arellano
Copy link
Contributor

commented May 9, 2019

Problem

To improve the UX of V2 rules, there are certain times that we would like to use colors in the output, just as we do in V1.

However, we also need some way to respect the global flag --no-colors / --colors.

Solution

Add safe color methods to the Console and MockConsole classes. These take a parameter for whether colors may be used, which in the former case get initialized by the global options.

Console rule users would then use colors like this: console.write_stdout(console.green("Pretty colors!")).

Alternative API considered: stripping output

Instead of defining color methods on the Console class, we could have left the callers to use the colors library normally, then changed Console.write_stderr() et al. to call colors.strip_colors() on the payload.

This reduces the risk of people in the current approach accidentally directly using colors rather than the console methods. It also allows us to keep the definition of Console a bit smaller.

However, the idea of first adding color codes then intentionally stripping them away is awkward and it was decided that this was not worth it.

Result

Colors can now be safely used in V2, which unblocks #7676.

The v2 test rule was also updated to print in red "Tests failed" upon any failures.

Screen Shot 2019-05-09 at 8 53 41 AM

@illicitonion
Copy link
Contributor

left a comment

Looks reasonable to me, but this definitely isn't my area of expertise.

Show resolved Hide resolved tests/python/pants_test/engine/util.py Outdated

@illicitonion illicitonion requested a review from blorente May 9, 2019

@stuhood

stuhood approved these changes May 9, 2019

Copy link
Member

left a comment

Thanks!

I expect that MockConsole and Console can actually be merged at some point, but just keeping them shaped the same way is fine for now.

@@ -145,9 +147,10 @@ def remove_locations_from_traceback(trace):
class MockConsole(object):
"""An implementation of pants.engine.console.Console which captures output."""

def __init__(self):
def __init__(self, use_colors=True):

This comment has been minimized.

Copy link
@stuhood

stuhood May 9, 2019

Member

Maybe for a followup: defaulting this to false (or using false explicitly in tests) would avoid needing to strip things in most cases.

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano May 9, 2019

Author Contributor

Ack. Realized it was important parametrize this and to not hardcode for exactly that reason.

I'm thinking that we'll want callers of this to explicitly say that they don't want colors—to opt out, rather than opt in.

Usually for unit tests, we will want to check that colors are being rendered properly (iirc we do this for V1 in some places), so I think it's good to default to this here.

@Eric-Arellano Eric-Arellano merged commit fd3ff0c into pantsbuild:master May 9, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Eric-Arellano Eric-Arellano deleted the Eric-Arellano:v2-colors branch May 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.