opengisch / pum

Postgres Upgrades Manager
GNU General Public License v2.0
30 stars 7 forks source link

Remove the DeltaPy.variable methods #61

Closed elemoine closed 5 years ago

elemoine commented 5 years ago

This PR suggests to remove the DeltaPy.variable methods. To access a variable from a child class one can just use the self.variables dict. I don't think there's a need for more abstraction here.

I also added a commit to rely on psycopg2 to pass parameters to the SQL query in the delta_1.1.0_delta_with_param.py delta file in the tests.

Relies on #59, which should be merged first.

Fixes #60.

elemoine commented 5 years ago

Isn't variable (not variables), the duplicated one.

Yep. Did I say otherwise?

Rather than removing these methods, why not just removing the default value for the default value (default_value=None) ?

What does "removing the default value for the default value" mean? :-)

I don't see the value for a variable function. There's a variables property already, providing access to the all the variables. Why increasing the API surface with an additional function. The person who creates a child class of DeltaPy should know how to use a dict.

3nids commented 5 years ago

I meant to have only variable(name, default_value) and not variable(name, default_value=None). I have no strong opinion here, I understand your argument, but I like the simple methods to call.

elemoine commented 5 years ago

I meant to have only variable(name, default_value) and not variable(name, default_value=None). I have no strong opinion here, I understand your argument, but I like the simple methods to call.

My opinion is that we have properties for everything else: delta_dir, delta_dirs, current_db_version, upgrades_table and pg_service, so I think it makes sense to expose a variables property as well. And with a variables property exposed there is no need for additional methods. variables is a dict and people creating child classes of DeltaPy should know how to manipulate dict. For example they can use self.variables.get('var_name', default_value) to get the value of a variable with a default value if the key does not exist in the variables dict.

3nids commented 5 years ago

Ok, I will just have to adapt another repo before merging this.