lightsheet-team / lightsheet

GNU Lesser General Public License v3.0
1 stars 1 forks source link

Implement SheetHolder and cross-sheet referencing #117

Closed Fluglow closed 3 months ago

Fluglow commented 3 months ago

Closes #101

SheetHolder

Sheet / ExpressionHandler

Lightsheet

Refactoring

Yousif-FJ commented 3 months ago

Here is my opinion on this and why I think this is problematic:

sheet holder containing cellData

My suggestion, would be to go back to something similar to the structure we agreed on:

I know this might be a big change from what you did, but I have to give my opinion.

Fluglow commented 3 months ago

@Yousif-FJ I think your proposals should be implementable from this. I chose this easier route since separating logic from Sheet proved to be quite tricky, but it should be easier from this stage. The main issue was that we want to keep most methods private in Sheet, and separating logic easily leads to having to call these internal methods from elsewhere.

SheetHolder containing cellData is akin to the structure of having a separate DataManager, but they are just combined now. I again see the benefit of holding cellData separate from Sheet, since cell information doesn't need to be indexed by anything else (holding it in Sheet is essentially indexing by SheetKey). Cell objects are also cross-sheet by nature as they can hold keys from multiple different sheets, so having them in a broader context seems more correct.

rafinutshaw commented 3 months ago

This excellent work @Fluglow

rafinutshaw commented 3 months ago

When i try to refer to a sheet that does not exist. it show show an error just like in excel. instead it is clearing the field