Merging fix of Spreadsheet/ExcelCompiler, updated tests, Attempt 2

[bradbase]

No description provided.

[Brad-eki]

@danielsjf Please take a look and see if this is the outcome you had intended.

@danielsjf and @brianmay, tests are passing and both examples are working.

[danielsjf]

@Brad-eki Thanks a lot for these changes. With over 50 files changed it is hard to check everything. It would have been a bit easier if you would have split the doc changes into another pull request ;-) However, when going through the code I had a few questions/comments:

• I saw you reverted most of the changes in the new function names (e.g. set_value -> cell_set_value). Was this on purpose?
• I do like the move to add a specific function for the fromfilename function. However, to be consistant I would name it from_filename (also the official guidance from PEP).
• I also saw you reintroduced the gen_graph function that is required to use the function. In my opinion this introduces an extra step which could be easily included under the from_filename function. Of course this is open for debate.
• At the moment it seems that so many changes in Spreadsheet are rolled back that it almost seems as if an older version was used for it. This includes small changes such as renaming variables (e.g. val -> value).
• I saw you did a lot of cleaning in utils.py. This certainly improves the readability a lot!

I'll have another look tonight.

[Brad-eki]

@danielsjf

• At the moment it seems that so many changes in Spreadsheet are rolled back that it almost seems as if an older version was used for it. This includes small changes such as renaming variables (e.g. val -> value).

I have used an older starting point. I began with 0.0.30 as this was the last version I know I had been able to use which was using both Spreadsheet and ExcelCompiler. I then cherry picked each change, running all tests and both examples for each change.

A sledgehammer approach, probably. But things weren't working and raising bizarre errors I needed to work it though. Apologies for the complex PR.

If these changes need to be packaged in a different form, then we can probably arrange that.

• I saw you reverted most of the changes in the new function names (e.g. set_value -> cell_set_value). Was this on purpose?

The approach I took to refactor Spreadsheet/ExcelCompiler was to move ExcelCompiler functionality but leave as much alone as possible. Now that functionality has been moved re-naming those function names can be done easily and comprehensively (eg; including the examples) and not get wrapped up in a major change. I didn't do it in this particular PR as I got to 3am and decided that stuff will be easy sometime later.

• I do like the move to add a specific function for the fromfilename function. However, to be consistant I would name it from_filename (also the official guidance from PEP).

Cheers. I am not a great fan of this particular solution, I see it as a hack - it's just a better one than an If in a constructor. I answer why in the next dot point.

I'm not fussed on what it's called providing the use is clear and it actually does the job you expect when you call it.

• I also saw you reintroduced the gen_graph function that is required to use the function. In my opinion this introduces an extra step which could be easily included under the from_filename function. Of course this is open for debate.

The short reason is that this is currently the way that code gets used - it's part of the existing design. To remove the gen_graph step actually requires significant code change and not just refactor. It would be a very big challenge to attempt both kinds of change at the same time.

The long reason is that Spreadsheet and ExcelCompiler have distinct and different purposes. Bringing the ExcelCompiler functionality into Spreadsheet is creating a second purpose for Spreadsheet which it hadn't been designed to do. This is breaking some 101's of object oriented concepts. The clearest indicator of such a break is that we need a second constructor where the signature bares little resemblance to the original/main one. Unlike Java, Python doesn't support overloaded constructors so one "elegant"(/?) solution is the one I've put forward. I don't like the multi-constructor solution for this situation, but the only other way to solve this is to re-architect the project to adopt the extra use-case now found in Spreadsheet. If Spreadsheet is to have the ExcelCompiler functionality I think it's a good move to refactor, see what that means, and then do the re-write (eg; what we are doing). If we were to refactor and re-write Spreadsheet in isolation from the rest of the project the only outcome is to break the rest of the project in a very complex way (eg; the situation we had?).

• I saw you did a lot of cleaning in utils.py. This certainly improves the readability a lot!

Cheers :D

[Brad-eki]

I'll re-package this and see how we go.