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

Fix hostname colorize #354

Merged
merged 3 commits into from Apr 21, 2018
Merged

Fix hostname colorize #354

merged 3 commits into from Apr 21, 2018

Conversation

comagnaw
Copy link
Contributor

@comagnaw comagnaw commented Feb 4, 2018

@b-ryan

This PR fixes issue #353 and adds a unittest. The fix is adding one line to getOppositeColor which needs to convert the rgb values to float before sending to rgb_to_hls. This stackoverflow article helped narrow down the solution.

This issue only occurred on hostnames that happened to hash to a value in ranges that would be computed incorrectly by rgb_to_hls. In most instances the returned opposite values from getOppositeColor would be a -1 in the r,g, or b position. The unittest added also caught, although is not asserting, ZeroDivisionError exceptions that occurred in rgb_to_hls.

======================================================================
ERROR: test_rgb_input_get_opposite_not_negative_2_0x20000 (color_compliment_test.getOppositeColorTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/me/.pyenvs/powerline-dev/lib/python2.7/site-packages/parameterized/parameterized.py", line 392, in standalone_func
    return func(*(a + p.args), **p.kwargs)
  File "/Users/me/repos/src/github.com/comagnaw/powerline-shell/test/color_compliment_test.py", line 42, in test_rgb_input_get_opposite_not_negative
    self.assertNotIn(negative, getOppositeColor(*test_input), u'{0:#08x} returns negative number in rgb tuple'.format(int(name,16)))
  File "/Users/me/repos/src/github.com/comagnaw/powerline-shell/powerline_shell/color_compliment.py", line 16, in getOppositeColor
    hls = rgb_to_hls(r,g,b)
  File "/usr/local/Cellar/python/2.7.14_2/Frameworks/Python.framework/Versions/2.7/lib/python2.7/colorsys.py", line 77, in rgb_to_hls
    s = (maxc-minc) / (2.0-maxc-minc)
ZeroDivisionError: float division by zero

Unfortunately, the unittest could not realistically be written to test 2^24 values that my be computed against a hostname (I tried....but did not have enough memory or patience to even build/run the parameterized tests. I did run a unittest that was not parameterized and got a successful result against the 2^24 but any failures discovered by that unittest would be hard to narrow down, given it was a one and done unittest...it did take a long time but required little to no memory). The test is written to test 768 values via parameterized package, which was added to the requirements-dev.txt. If you uncomment the conversion to float in getOppositeColor, you will get 192 assertions and 3 errors.

@b-ryan
Copy link
Owner

b-ryan commented Feb 24, 2018

Hey @comagnaw thanks a lot for the thorough issue and PR. I will try to get through all of what you wrote and internalize everything soon. I was not the maintainer / owner of this project when the colorize code was written so I don't know how it works.

You mentioned in the issue that maybe the colorize option should be a theme configuration. I tend to agree. I am not the fondest of the colorize option. My understanding is it serves as a flag for when you are SSH'd into another machine. There is the ssh segment nowadays though, so I wonder if either people don't know that exists or if there is some other function being served by the colorization.

@b-ryan
Copy link
Owner

b-ryan commented Apr 21, 2018

Finally got around to understanding this whole problem. Your solution looks good to me. It changes colors for any users that are already using this option, but this looks like the proper solution. Thanks for the thorough issue and fix.

@b-ryan b-ryan merged commit 6eff210 into b-ryan:master Apr 21, 2018
b-ryan pushed a commit that referenced this pull request Apr 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants