Error adding two Color objects

Issue #927 resolved
Eric Kinoshita
created an issue

TLDR: Color(rgb='FF00FF00') + Color(rgb='FF0000AA') raises TypeError: expected <type 'long'> - mokey patch below.

I've been using a lot the style_range function (as presented on http://openpyxl.readthedocs.io/en/latest/styles.html#styling-merged-cells). And it raises weird TypeErrors coming out of lines like the following:

cell.border = cell.border + top

That exception can be easily reproduced by doing this:

>>> from openpyxl.styles import Border, Side
>>>
>>> Border(top=Side(style='thin', color='FF00FF00')) + Border(top=Side(style='thin', color='FF0000FF'))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "my_venv/local/lib/python2.7/site-packages/openpyxl/descriptors/serialisable.py", line 208, in __add__
    vals[el] = a + b
  File "my_venv/local/lib/python2.7/site-packages/openpyxl/descriptors/serialisable.py", line 208, in __add__
    vals[el] = a + b
  File "my_venv/local/lib/python2.7/site-packages/openpyxl/descriptors/serialisable.py", line 211, in __add__
    return self.__class__(**vals)
  File "my_venv/local/lib/python2.7/site-packages/openpyxl/styles/colors.py", line 86, in __init__
    self.indexed = indexed
  File "my_venv/local/lib/python2.7/site-packages/openpyxl/descriptors/base.py", line 69, in __set__
    value = _convert(self.expected_type, value)
  File "my_venv/local/lib/python2.7/site-packages/openpyxl/descriptors/base.py", line 59, in _convert
    raise TypeError('expected ' + str(expected_type))
TypeError: expected <type 'long'>

I did some digging and I found out that the original error happens when it tries to add two colors:

>>> from openpyxl.styles import Color
>>>
>>> Color(rgb='FF00FF00') + Color(rgb='FF0000AA')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "my_venv/local/lib/python2.7/site-packages/openpyxl/descriptors/serialisable.py", line 211, in __add__
    return self.__class__(**vals)
  File "my_venv/local/lib/python2.7/site-packages/openpyxl/styles/colors.py", line 86, in __init__
    self.indexed = indexed
  File "my_venv/local/lib/python2.7/site-packages/openpyxl/descriptors/base.py", line 69, in __set__
    value = _convert(self.expected_type, value)
  File "my_venv/local/lib/python2.7/site-packages/openpyxl/descriptors/base.py", line 59, in _convert
    raise TypeError('expected ' + str(expected_type))
TypeError: expected <type 'long'>

I found that the Color class inherits the despcriptors.serializable.Serialisable.__add__ method, and its implementation does not suit to the Color class. I'm using the following monkey patch and it seems to work fine:

def _new_color_add(self, other):
    return other if isinstance(other, Color) else self

Color.__add__ = _new_color_add

Comments (7)

  1. CharlieC

    I've stopped the exception from happening. As it's not really clear how this should be handled the original colour is returned if two are added. If a different colour is desired then reassignment is the way to go.

  2. Eric Kinoshita reporter

    Actually, I think that the "other" color should be considered.

    Take these snippets from style_range function as an example:

    cell.border = cell.border + top
    # ...
    cell.border = cell.border + bottom
    # ...
    l.border = l.border + left
    r.border = r.border + right
    

    The top, bottom, left and right variables are set from values retrieved from the border kwarg. And I would expect that the new values overwrite the old ones.

    If adding colors always consider the left side (eg self) it sounds like once you set a color it becomes permanent.

    That's why I use return other if isinstance(other, Color) else self in my patch. I think your change would be better like this:

    def __add__(self, other):
        if isinstance(other, Color):
            return other
        else:
            super(Color, self).__add__(other)
    

    Or something like that, prioritizing the other side for overwriting.

    Thanks, by the way.

  3. CharlieC

    You're conflating two separate ideas: how should colours be added together and composition of complex objects. As it's not really possible to define how colours should be added together the implementation does nothing and no longer raises an exception. In addition to the problems of adding red to green there are the different colour models to take into consideration. As the behaviour is essentially undefined the only safe thing is to do nothing and let the user decide. The alternative would be a custom exception.

    Working backwards from the border example is asking for special behaviour in a single case. Other cases might expect different behaviours (colours crop up in many different places). Styling ranges is a problem because of a poor description in the specification.

    The issue is now closed. Any further discussion should either be on the mailing list or via a pull request.

  4. Log in to comment