Updated DataValidation.cells to always be a set.

#179 Merged at fb05881
Repository
gar_thompson
Branch
2.4
Repository
openpyxl
Branch
2.4
Author
  1. g
Reviewers
Description

The expand_cell_ranges method changed cells from a set to a list. This meant that a call to add() failed if called after sqref().

Comments (5)

  1. CharlieC

    Can you provide some more context for this? Also, please note that SQREF in XML is a list and we actually need to change the code to allow for an arbitrary list of cells and cell ranges: A1 A3:A5 A7. I think order should be preserved.

  2. g author

    There's currently an issue in that the DataValidation constructor initialises the cells attribute to a set and the add methods treats it as such (throwing an error if cells is a list). The sqref setter sets cells to a list (via expand_cell_ranges). If you try to call add(...) after the sqref setter has been called it will throw an error.

    I thought I'd take a stab at one approach to fixing it. Changing the constructor and add methods to use list would be equally valid, and from your comments it sounds like it would be a better approach.

    The only downside with the list approach is that it's not as quick to check if a cell has a particular data validation applied. In my particular use-case, this is less than ideal as I'm trying to extend certain data validations across all rows / columns in potentially large worksheets. The check to see if a particular cell has any data validation applied will get incrementally slower as the data validations are extended to more cells. Perhaps there's a better approach to this problem - e.g. storing a back-reference to data validations on the cell?

    1. CharlieC

      sqref is supposed to be called only as part of the serialisation. It should really be a descriptor that manages a sequence of cell or cell ranges. If you look at the history of the code you'll see that I've been changing it to use the more declarative approach while trying to preserve any existing API but it really needs to go the whole way, plus support discrete cells. The ordering only needs to happen when serialising or inspecting so the conversion to a list when reading the XML is not essential.

  3. CharlieC

    To be clear I think the original change was probably okay. No need for an additional for test but tests for the set must also work with Python 2. You can use tox to check this.