Provide better error message/type when optimized_write=True workbook is attempted to be modified

Issue #342 resolved
Péter Zsoldos created an issue

Tried with 2.0.4

from openpyxl.cell import get_column_letter
from openpyxl.styles import Style, NumberFormat
from openpyxl.workbook import Workbook


wb = Workbook(optimized_write=True)
ws = wb.create_sheet()
ws.append(['Pro', '15 %'])
ws['B1'].style = Style(number_format=NumberFormat('0%'))

Outputs

Traceback (most recent call last):
  File "x.py", line 9, in <module>
    ws['B1'].style = Style(number_format=NumberFormat('0%'))
  File "/var/www/BIS-trunk/venv-latest/lib/python2.6/site-packages/openpyxl/cell/cell.py", line 423, in style
    return self.parent.set_style(self.coordinate, new_style)
  File "/var/www/BIS-trunk/venv-latest/lib/python2.6/site-packages/openpyxl/worksheet/worksheet.py", line 430, in set_style
    self._styles[coordinate] = self.parent.shared_styles.add(style)
TypeError: list indices must be integers, not str

While it's true it is an end user error, it took me some time to figure out the problem was that I was attempting to modify an optimized write workbook, which of course cannot be modified by definition.

Would have been nice to get a CannotModifyOptimizedWorkbookError raised instead

Comments (6)

  1. CharlieC

    Starting with 2.1 the flag will be write_only instead of optimized_write which should be a bit more helpful. We will be refactoring the code so that methods that are not supported for this kind of worksheet will simply not be available. However, I think there will always be edge cases that we can't manage and I think this may be one.

  2. Péter Zsoldos reporter

    Changing the flag would bring clarity. I would propose write_only_once as an alternative name for the flag, but I have to admit while it's more explicit, it doesn't read well.

    AttributeError: WriteOnlyWorksheet has no attribute <blah> also sounds like a great improvement!

    However in this case it's not the Worksheet that is effected, but rather the Cell. I don't know the details of the planned refactoring, but in the spirit of "explicit is better than implicit" I would hope for clear errors. One neat way would be if WriteOnlyWorksheet raised a NotImplementedError when the code tries to access any of its cells (or any other property for that matter).

  3. CharlieC

    The refactoring will allow Cell objects to be used in write_only environments. This will reduce the current code duplication when converting to Excel and allow the same interface for in ws.append() for normal and write_only worksheets - this was broken in 2.0 by allowing Style and Comment objects to be passed in dictionary in write_only, where normal worksheets use dictionaries for coordinate, value combinations.

    However, if we use cells for this then their style and comment attributes have to be writeable. In the case of styles this will raise an error because styles are immutable, changes to comments would fail silently because although the cell object might have changed, it's already been serialised. The fix for this will be to unpick the inheritance: DumpWorksheet inherits from Worksheet and thus pretends that you can read cells, when you can't actually do this.

    Style management will be removed from Worksheets - they are actually Workbook variables - which will allow for a more consistent interface so that the error you're currently seeing won't happen.

  4. Log in to comment