MathNya / umya-spreadsheet

A pure rust library for reading and writing spreadsheet files
MIT License
239 stars 41 forks source link

breaking change #115

Closed cstkingkey closed 10 months ago

cstkingkey commented 1 year ago

ef3f99bee47115285dba223e35691f6933e196af and following commits change the logic of set_value_from_string, which break code relying on it and cause data loss.

Please rename the functions to avoid it.

cstkingkey commented 1 year ago

@john-dc252

john-dc252 commented 1 year ago

image Hello @cstkingkey,

Is this the change that you mean? Do you mean that we should rename set_value_from_string to something else? Or should we rename it back to set_value?

cstkingkey commented 1 year ago

image Hello @cstkingkey,

Is this the change that you mean? Do you mean that we should rename set_value_from_string to something else? Or should we rename it back to set_value?

set_value_from_string is renamed to set_value_string, while set_value is renamed to set_value_from_string. I use set_value_from_string, so it leads to data loss.

since version 0.9 is published, both functions need to be renamed to different names to force downstream developers to use correct functions. And I'm afraid that all relevant functions need a review.

john-dc252 commented 1 year ago

Hello @cstkingkey ,

While I did mention these changes in my PR, they are not mentioned in the readme change log for 0.9.

Would updating the readme to mention this change not be enough?

cstkingkey commented 1 year ago

Hello @cstkingkey ,

While I did mention these changes in my PR, they are not mentioned in the readme change log for 0.9.

Would updating the readme to mention this change not be enough?

It would be better to change the names. It can be enough to mention this change in the readme. Either is acceptable to me, as I already paid the cost.

john-dc252 commented 1 year ago

@MathNya

MathNya commented 1 year ago

@john-dc252 @cstkingkey We have checked the situation. I will try to handle this by renaming set_value_from_string to set_value. This is more backward-compatible and less uncomfortable with the name set_value.