lightsheet-team / lightsheet

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

Improve handling of empty cells #100

Closed Fluglow closed 6 months ago

Fluglow commented 6 months ago
Yousif-FJ commented 6 months ago

I don't think there should be a allowEmpty flag. I think it is fine to have an empty cell, they are only deleted when the user explicitly deletes them. It just adds complexity with very little benefit. As the famous quote says, "The best code is no code".

It is not the end of the world if a cell exists for no reason, If it becomes too much of an issue later, we can have a function that purges them on save.

Fluglow commented 6 months ago

I don't think there should be a allowEmpty flag. I think it is fine to have an empty cell, they are only deleted when the user explicitly deletes them. It just adds complexity with very little benefit. As the famous quote says, "The best code is no code".

It is not the end of the world if a cell exists for no reason, If it becomes too much of an issue later, we can have a function that purges them on save.

The issue I have is that the core should simply never have empty cells. They serve no purpose and waste memory, and it doesn't make sense that setCell can lead to such unexpected outcomes if the caller isn't aware of the nuance. The only use case I can think of is internal use, where momentarily we create an empty cell to populate its references after. Is there any other use case? If not, I would rather exclude this behavior altogether from setCell and create a private method for this.

Yousif-FJ commented 6 months ago

The issue I have is that the core should simply never have empty cells. They serve no purpose and waste memory, and it doesn't make sense that setCell can lead to such unexpected outcomes if the caller isn't aware of the nuance. The only use case I can think of is internal use, where momentarily we create an empty cell to populate its references after. Is there any other use case? If not, I would rather exclude this behavior altogether from setCell and create a private method for this.

I see what you mean, my concern with the 'never have empty cells' policy is that I'm more worried about needing to write a little bit of code just to make sure that we don't leave an empty cell, like needing to write a condition on expression evaluation to ensure we never have empty cells.

So yeah I can't think of any other way and leaving empty cells might not be a good solution as you mentioned. So I guess you can make a private function for that, but also making sure that those empty cells are deleted.

Fluglow commented 6 months ago

Since we can't access the private method from ExpressionHandler, I decided to change the structure a bit to initialize the referenced cells in processEvaluationReferences (previously handleCellReferenceChanges). I modified ExpressionHandler::evaluate to return an index position instead of keys for references.

like needing to write a condition on expression evaluation to ensure we never have empty cells.

This shouldn't be an issue as long we do other stuff correctly. Cleanup should always be handled by internal Sheet methods, abstracting it away from other components.