qrilka / xlsx

Simple and incomplete Excel file parser/writer
MIT License
130 stars 64 forks source link

Cell Range dataValidation problem #156

Closed flhorizon closed 2 years ago

flhorizon commented 2 years ago

Hello,

I'm working on a feature involving cell range data validation.

As it is in very version I've checked (0.7.2 we use to 1.0.0.1) ValidationTypeList only supports plain list dataValidation.

Cell range validation is basically a < dataValidation type="list" ..> too, with (unfortunately) nearly no difference with a plain list.

The only difference is that instead of a quoted comma-separated list as <formula1>, it's rather an unquoted cellref expression:

        <dataValidation operator="between" showDropDown="false" sqref="D2:D1000" type="list">
            <formula1>
                MyValidationSheet!$C$2:$C$18
            </formula1>
               <dataValidation/>

I'd very much like to contribute a decent fix (more than a kludge I could get away with using my own changes 😛 )

And for this I'd like to discuss the best way (if there's any) for this to be a patch/minor bump rather than a major.

What could be a major change I guess (but the cleanest?) would be changing DataValidationType like:

data DataValidationType =
   -- ...
   |  ValidationTypeList   ListOrRangeExpression

data ListOrRangeExpression = ListExpression [Text] | RangeExpression Range  -- i.e. CellRef

As for preventing breakage, that's more kludge-y but having an heuristic detecting a single cellref expression in instance ToElement DataValidation for ValidationTypeList to determine whether to comma-join+quote (plain list) or leave it as is (cellref expression).

Do you think we could make a patch like that ? I don't mind working on both a patch and a proper, major change

qrilka commented 2 years ago

Hi @flhorizon Any suggestion are welcome but I'm not quite sure what you propose as a "patch" here. As Linus says "show me your code" :) Also @emilaxelsson added data validation in #67 so probably he could also add his 5 cents.

flhorizon commented 2 years ago

Hi @qrilka :)

Here's what I have so far, working for me:

https://github.com/flhorizon/xlsx/pull/1

I've tried to be as conservative as possible, so as not to change the contents of the list of ValidationTypeList [Text] for the existing use case.

As current lib users aren't using cell range validation, it shouldn't break anything for them.

Unless.. they are and didn't care until now that it was broken when parsing a file 🤔

(we're currently using 0.7.2 but it doesn't seem hard to port to later majors)

flhorizon commented 2 years ago

When I have a bit of spare time I'll implement the proposal extending the param type of DataValidationTypeList over 1.0.0.x

I guess there's no way to express in semver its backporting to 0.7.x and 0.8.x 😄

I'll admit the one with the formula header linked above (https://github.com/flhorizon/xlsx/pull/1) is a bit... lousy and just to sidestep upgrading my team's project deps to 1.0.0.1

qrilka commented 2 years ago

Sure, take you time, small not though - I'm on vacation for a couple of weeks so could be not that fast with a response, otherwise - any contribution is welcome

flhorizon commented 2 years ago

Hello,

I've finally ported my cellrange DV. code to master

https://github.com/qrilka/xlsx/pull/157

I've also needed support for absolute coordinates along the way so I've also added it too