meksor
commented
1 month ago @glatterf42 OK so i think the currently proposed changes (removing the commented base model) are good. I also think the rest of the facade layer could also benefit from something like this. Lets figure the @property part out later, for now just explicitly having them in every leaf class is okay imo.
Since we are now not depending on sqlalchemys session-synchronization anyway (changing something in a model with id=1 will change it in all models referencing that row) it would also be okay to just reassign the properties (maybe automatically) to the facade class from the api or db model, thereby completely removing the whole property thing for most stuff.
For #94, @danielhuppmann asked in the
corelayer: "Why doesParameternot inherit fromTable"? And sure enough, these optimization items share a lot of common functionality. So before moving on toVariableandEquation, for which this will be the case even more so, I wanted to take another look at this.@meksor, I expect you'll be the one to take a look at this. Sorry, this description might be a bit lengthy.
Initially, I interpreted the question as for the DB layer, and how to handle inheritance (or rather Composition) there has been recently discussed in #82. Instead, this PR focuses on the core layer, where we also have two classes per optimization item that could potentially benefit from higher reusability (to follow the DRY principle): e.g., the
Table(mostly a collection of data attributes) and theTableRespository(functions to handleTables). For theTables themselves (and similar), we are currently using Python's property to make data attributes accessible in a controlled way. Unfortunately, thesepropertys don't mix well with inheritance. This article describes how to still make it work for thegetterpart, but since we would still have to repeat the same number of lines in the data files, I'm not sure it's worth the effort here. What we stand to gain is a separation of functionality and placeholder code, it seems to me. So when we want to make changes to how e.g.datais returned from an optimization model, we would just need to change one place and all others would inherit these changes. However, code repetition would not be decreased and understanding the models might become more complicated, so I've put this on hold for now. ForTableRepository, there is luckily a quite straightforward way: we can employ the same approach as in the DB layer and create anOptimizationBaseRepositoryin the core layer that can be extended withCreator,Retriever, etc as needed. This seems to work well and every optimization item repo then just needs to clarify which_model_typeand_backend_repositoryit is interested in. For better understanding, I wanted to type hint the_backend_repositoryin theOptimizationBaseRepository. However, we didn't already have an appropriate object to do so. That's why I have created aBackendBaseRepositoryinabstract.optimization.base, which simply announces how the actualTableRepositoryin the DB layer is going to look. I'm not sure if this is the correct location; I've usually worked with abstract models to type hint API/DB layer functionality, but I also don't know where else it would live.This is the point where I'm not sure I'm naming things correctly anymore. I started from
Table, which already hasTable.data, which is arguably the largest part of shared functionality, at least in the DB layer, and which is missing fromIndexSetandScalar. So these two items could not use theOptimizationBaseRepositoryas it is now, which seems like a misnomer. Maybe the data functionality needs to be further separated (allowing us to DRY even more parts of this layer) or maybe we find another name? The same applies forBackendBaseRepository.