You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
The main problem is that we rely on state stored in this class instance to build the site config and database. In many functions, instance variables are accessed that are set in another function, which can lead to bugs as the data is being set in one place and used in another, without an explicit connection between the two (implicit dependencies). The order in which these functions are run is important and will give errors if changed.
I think the function save_config() is a great place to start and example for my idea:
by replacing the var=self.site_attrs['var'] dictionary variables with stateless functions that return a(n) (Optional) model that is read from the configuration file: read_sfincs_config(config_file) -> Optional[SfincsModel] ....
In this function, we can do some checks only on the part that is relevant for this part of the config: not present -> we can return None, contains errors-> we can raise, otherwise -> return the created model.
This way, the order of functions doesnt matter, since we just call read_site_config(), which will call read_sfincs_config(), which can call more functions that read even smaller parts from the config_file/config_dict.
Some small cherries on top:
much easier to test (1 function with 1 input and 1 output, instead of many having to make sure you use the correct function ordering).
easy to add additional configs to, as we can just add another function.
All functionality related to a part of the config will be in 1 function
There is of course the part of actually creating the database and copying/saving stuff to the correct place. This should be done in a similarly structured function like setup_data(SiteModel) that takes an an argument the validated SiteModel, which then calls functions like setup_sfincs_data(SfincsConfigModel) etc with the same nested functions idea.
I think it would be best to merge this PR and open a new issue (I dont mind writing it) for these ideas and improvments since it is not a negligible amount of work. Probably even be for after the beta release.
The text was updated successfully, but these errors were encountered:
panosatha
changed the title
Very nice improvements, especially using the class constructors! Makes it much more readable and maintainable.
Database builder refactor
Feb 3, 2025
The main problem is that we rely on state stored in this class instance to build the site config and database. In many functions, instance variables are accessed that are set in another function, which can lead to bugs as the data is being set in one place and used in another, without an explicit connection between the two (implicit dependencies). The order in which these functions are run is important and will give errors if changed.
I think the function
save_config()
is a great place to start and example for my idea:by replacing the
var=self.site_attrs['var']
dictionary variables with stateless functions that return a(n) (Optional) model that is read from the configuration file:read_sfincs_config(config_file) -> Optional[SfincsModel] ...
.In this function, we can do some checks only on the part that is relevant for this part of the config: not present -> we can return None, contains errors-> we can raise, otherwise -> return the created model.
This way, the order of functions doesnt matter, since we just call
read_site_config()
, which will callread_sfincs_config()
, which can call more functions that read even smaller parts from the config_file/config_dict.Some small cherries on top:
There is of course the part of actually creating the database and copying/saving stuff to the correct place. This should be done in a similarly structured function like
setup_data(SiteModel)
that takes an an argument the validated SiteModel, which then calls functions likesetup_sfincs_data(SfincsConfigModel)
etc with the same nested functions idea.I think it would be best to merge this PR and open a new issue (I dont mind writing it) for these ideas and improvments since it is not a negligible amount of work. Probably even be for after the beta release.
Originally posted by @LuukBlom in #668 (review)
The text was updated successfully, but these errors were encountered: