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

Add CLI option to disable colorama #118

Merged
merged 13 commits into from
Apr 1, 2016
Merged

Add CLI option to disable colorama #118

merged 13 commits into from
Apr 1, 2016

Conversation

dougthor42
Copy link
Contributor

This PR adds a -W or --nowindows command line argument that, when present, disables windows support via Colorama.

This is useful for some CI cases, such as GitLab. See #117.

@dougthor42
Copy link
Contributor Author

  • I'm not super happy with --nowindows and no_windows, but I couldn't come up with anything better.
    • disablewindowssupport is much more descriptive, but too long and hard to read without CamelCase

@CleanCut
Copy link
Owner

You've got some interesting AppVeyor output weirdness and a test failure to deal with. ;-)

@dougthor42
Copy link
Contributor Author

Yup, just saw those. Poop.

@dougthor42
Changed testNoWindowsFalse to assert that the stream is a subclass of…
c74c114 
… colorama's StreamWrapper

+ also fixed some typos and updated docstrings.
@dougthor42
added override_appveyor to testNoWindowsFalse.
007553e 
+ removed commented-out line of code.
@dougthor42
Renamed vars from "nowindows" to "disablewindows". Updated GreenStrea…
99dc4af 
…m logic to be more clear.

+ I was confusing myself with the logic and Bad Things (tm) were happening.
@dougthor42
Added disable_windows arg to the GreenStream constuctor in cmdline.ma…
4b9bc28 
…in().

+ This made things actually work when the cli arg was present. Horray!
@dougthor42
Renamed CLI arg '--nowindows' to '--disable-windows'
b0fbe3a
@dougthor42
Forgot to update cli-options.txt for the rename to '--disable-windows'
e4bc816
@dougthor42
Copy link
Contributor Author

It looks like Appveyor is printing some of the ANSI codes. I can't seem to recreate it on my end - when I run tests everything looks just fine.

(Working dir is the green project root (with README.md, etc. in it))

> pip uninstall green -y
> python setup.py sdist bist_wheel
> pip install dist\green-2.3.0-py3-none-any.whl
> green green -s 1 -vv -W      # correctly does *not* color things and *does* have ANSI codes displayed
> green green -s 1 -vv         # correctly colors things.
> python -m green.cmdline -s 1 -vvv -t green   # the same command that Appveyor runs

None of the 3 outputs display what Appveyor is showing. I'm using -s 1 because I'll intermittently get tests that hang on multiprocess stuff.

Also, the Travis Builds are failing but it looks unrelated to this PR.

dougthor42 and others added 3 commits February 28, 2016 11:29
@dougthor42
Seeing what happens to appveyor if I use green -W
f9aca77
@dougthor42
Attempting to debug Appveyor's odd output. Temporarily removed disabl…
3a20dfe 
…e_windows from GreenStream

+ Appveyor is displaying "80" and "24" between some, but not all, tests.
@dougthor42
Manually reverted previous two commits.
6c6d09e 
+ Removed `green -W` line from appveyor.yml
  + Now it's back to running only 1 set of unittests
+ Re-enabled `disable_windows` logic in GreenStream.
@dougthor42
Copy link
Contributor Author

I took a look at the Appveyor build history. The odd "80" and "24" printed lines started between builds 273 (commit 51a709c) and 274 (commit 366f48e).

So I'm going to say "it wasn't me!" and breathe a sigh of relief 😆.

Unless you have any other comments on it, I'll stop development and you can merge at your leisure.

@CleanCut
Copy link
Owner

Okay, I'll review it soon as I can. No promises on the timeline, I'm in crunch time for my real job. ;-)

@dougthor42 dougthor42 mentioned this pull request Mar 2, 2016
Closed
@CleanCut CleanCut merged commit 18c20ab into CleanCut:master Apr 1, 2016
@dougthor42 dougthor42 mentioned this pull request Nov 4, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dougthor42 @CleanCut