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

Cell Range dataValidation problem #156

Closed
flhorizon opened this issue Jul 29, 2022 · 5 comments · Fixed by #157
Closed

Cell Range dataValidation problem #156

flhorizon opened this issue Jul 29, 2022 · 5 comments · Fixed by #157

Comments

@flhorizon
Copy link
Contributor

flhorizon commented Jul 29, 2022

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
Copy link
Owner

qrilka commented Jul 29, 2022

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
Copy link
Contributor Author

Hi @qrilka :)

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

flhorizon#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
Copy link
Contributor Author

flhorizon commented Jul 29, 2022

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 (flhorizon#1) is a bit... lousy and just to sidestep upgrading my team's project deps to 1.0.0.1

@qrilka
Copy link
Owner

qrilka commented Jul 30, 2022

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
Copy link
Contributor Author

Hello,

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

#157

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants