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

Un-unicode output for Windows CMD #120

Merged
merged 5 commits into from May 26, 2016
Merged

Un-unicode output for Windows CMD #120

merged 5 commits into from May 26, 2016

Conversation

@MinchinWeb
Copy link
Collaborator

MinchinWeb commented Apr 12, 2016

Fixes #119

@@ -23,3 +23,6 @@ def test_colorOutput(self):
self.assertTrue(issubclass(type(gs.stream),
colorama.ansitowin32.StreamWrapper))

def test_unicode_override(self):
pass

This comment has been minimized.

Copy link
@CleanCut

CleanCut Apr 12, 2016

Owner

Looking pretty good, except for this poor test here. ;-)

This comment has been minimized.

Copy link
@MinchinWeb

MinchinWeb Apr 13, 2016

Author Collaborator

It is a poor, sad test :(

I couldn't figure out how to get green to run a unittest testcase, and then validate what gets printed out. The actual testcase is nearly trivial...

class UnicodeTest(unittest.TestCase):
    def test1(self):
        print('\u03BB')  # lambda
        self.assertTrue(True)

Suggestions on how to go about this?

This comment has been minimized.

Copy link
@jayvdb

jayvdb Apr 15, 2016

Collaborator

The true test is to spawn subprocess of green running that test method, and check the output.

https://github.com/wikimedia/pywikibot-core/blob/master/tests/pwb_tests.py#L71

There all sorts of gotchas, especially when doing this on Windows, and only Python versions, resulting a mess like
https://github.com/wikimedia/pywikibot-core/blob/master/tests/utils.py#L771

However I suspect you'll only need to set PYTHONIOENCODING to utf-8 for the subprocess

@coveralls
Copy link

coveralls commented Apr 12, 2016

Coverage Status

Coverage decreased (-0.1%) to 99.894% when pulling c95ecc8 on MinchinWeb:fix-119 into 73872d9 on CleanCut:master.

@coveralls
Copy link

coveralls commented Apr 12, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling b985bc5 on MinchinWeb:fix-119 into 73872d9 on CleanCut:master.

@jayvdb
Copy link
Collaborator

jayvdb commented Apr 15, 2016

All of these "on_appveyor" conditions look like handling only the simple case of Appveyor, and dont really test using green on Windows because there is special logic for Appveyor only.

e.g. I expect that green > green.log on Windows will fail, because it is more like an Appveyor environment (i.e. no stdin, which Python uses to determine console encoding).

This is just a general comment, as I realise that this changeset isnt introducing "on_appveyor", but it is making it more "normal" by exposing it as an attribute.

# Ironically, AppVeyor doesn't support windows win32 system calls for
# colors, but it WILL interpret posix ansi escape codes!
on_windows = platform.system() == 'Windows'
on_appveyor = os.environ.get('APPVEYOR', False)
self.on_windows = platform.system() == 'Windows'

This comment has been minimized.

Copy link
@jayvdb

jayvdb Apr 15, 2016

Collaborator

this shouldnt be an instance variable. it should be a package level constant.

on_windows = platform.system() == 'Windows'
on_appveyor = os.environ.get('APPVEYOR', False)
self.on_windows = platform.system() == 'Windows'
self.on_appveyor = os.environ.get('APPVEYOR', False)

This comment has been minimized.

Copy link
@jayvdb

jayvdb Apr 15, 2016

Collaborator

likewise, I expect this should be a package level constant. I dont see how this env var could ever be expected to change during an execution of green.

Also, wherever it is placed, it should be a private attribute, as this is not something that should become part of the public green contract.

Created a unit test for MinchinWeb's pull request
@coveralls
Copy link

coveralls commented Apr 16, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 7f76c84 on MinchinWeb:fix-119 into 73872d9 on CleanCut:master.

@coveralls
Copy link

coveralls commented Apr 16, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling a9ebf91 on MinchinWeb:fix-119 into 73872d9 on CleanCut:master.

@MinchinWeb
Copy link
Collaborator Author

MinchinWeb commented Apr 16, 2016

@jayvdb I've reworked the 'flags' inside the module a little to try and make things more explicit.

The test failed on Travis because coverage timed out.

The test failed on Appvoyor on a test where colorama is supposed to strip ANSI codes. But the issue is something gets fed a str but is expecting a unicode. Is this an issue with Python 2.7? With colorama? With unidecode? I'm not sure. Part of the reason I moved to Python 3 was to avoid the str/unicode mess... Suggestions??

In any case, the tests do pass on my local machine.

@@ -167,6 +171,9 @@ def writeln(self, text=''):
def write(self, text):
if type(text) == bytes:
text = text.decode('utf-8')
# Compensate for windows' anti-social unicode behavior
if self._ascii_only_output:
text = unidecode(text)

This comment has been minimized.

Copy link
@CleanCut

CleanCut May 19, 2016

Owner

Sorry for the delay. I have been just absolutely swamped.

I suggest just changing line 176 to read:

text = unidecode(unicode(text))

That may be work and we could be done with this and merge it.

This comment has been minimized.

Copy link
@CleanCut

CleanCut May 19, 2016

Owner

Hmm, wait. Python 3 doesn't have the unicode() function anymore.

Since it's a Python 2-specific fix, I suppose we should make that condition:

if self._ascii_only_output:
    if platform.python_version_tuple()[0] == '2': # pragma: no cover
       text = unicode(text)
    text = unidecode(text)
Explicitely convert to Unicode for Python 2
@coveralls
Copy link

coveralls commented May 26, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 9aa6f36 on MinchinWeb:fix-119 into 73872d9 on CleanCut:master.

@MinchinWeb
Copy link
Collaborator Author

MinchinWeb commented May 26, 2016

@CleanCut: I've made the changes you suggested

@CleanCut CleanCut merged commit 6ed00be into CleanCut:master May 26, 2016
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@MinchinWeb MinchinWeb deleted the MinchinWeb:fix-119 branch May 26, 2016
@CleanCut
Copy link
Owner

CleanCut commented May 26, 2016

Included in the 2.4.2 release. Which was just released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.