search

vallettea / koala

Transpose your Excel calculations into python for better performances and scaling.
GNU General Public License v3.0
147 stars 59 forks source link

Proposal to clean Spreadsheet class #188

Closed danielsjf closed 5 years ago

danielsjf commented 5 years ago

I have a proposal ready in pull request #187 to merge the ExcelCompiler and Spreadsheet class as originally proposed in #64. This passess all the existing tests and works fine on my more complicated excel sheets.

I would like to go one step further and structure the functions in the Spreadsheet class better. As a first step, I would like to rename all the functions that actually work on cells as cell_xxx. This to differentiate with other functions that work on the Spreadsheet as a whole. Some notable changes would be reset_cell -> cell_reset, evaluate -> cell_evaluate, set_value -> cell_set_value ...

At the same time, I would also like to document these functions with docstrings.

All these functions would also get a similar structure. Now some take addresses, others take Cell objects and yet others use a combination with booleans to indicate what is what. The proposed structure is the following:

I would keep the backward compatibility intact for now and I will use deprication warnings to indicate future changes.

@vallettea, @brianmay, @Brad-eki, you seem the most active contributors so adding you in the discussion.

brianmay commented 5 years ago

These changes seem to be fine with me. Yes, backward compatibility is probably important. We can remove these deprecated functions at a later date in a major release.

danielsjf commented 5 years ago

This is done now.

Brad-eki commented 5 years ago

Hi Guys,

Apologies for being absent.

I have found an error in the logic for this refactor which has created a bug. That said, I support this change.

Currently gen_graph is not functionally backwards compatible. It used to return a new instance of Spreadsheet and now only modifies self which is causing problems with the expected, and documented, use case for the gen_graph method. #214

danielsjf commented 5 years ago

@Brad-eki, this should be solved with your latest commits and I will close the issue.