search

spine-tools / Spine-Database-API

Database interface to Spine generic data model
https://www.tools-for-energy-system-modelling.org/
GNU Lesser General Public License v3.0
6 stars 5 forks source link

alternative_id is now required by update_parameter_values() but the error message doesn't say so #168

Closed soininen closed 2 years ago

soininen commented 2 years ago

Semantics of update_parameter_values() seems to have changed such that it breaks client code: items passed to the method are now required to include alternative_id field. However, the exception raised in check_parameter_value() says that 'Alternative not found'. We should fix it to say 'Missing alternative identifier` instead.

manuelma commented 2 years ago

Sorry about that! I removed a lot of boilerplate recently but didn't anticipate that. The DBCheckMixin._manage_stocks method should pick the current element from the database so only updated fields are necessary... What did I miss?

soininen commented 2 years ago

It's just the error message that confused me when I noticed that FlexTool3 web interface's unit tests were suddenly broken.

Makes me actually wonder if the change was intended? Previoulsy spinedb_api would happily fall back to 'Base' alternative but now I get an exception. On the other hand, I'm OK with with having to provide an alternative id which forces me to take a stance regarding the fall back id.

soininen commented 2 years ago

I think I get it now: it's not the error message only, but we now require more information in items when calling update_parameter_values(). I'm not so happy with having to provide that redundant information when the parameter value can be identified solely by its id. Can we revert the change?

manuelma commented 2 years ago

I can fix this.

manuelma commented 2 years ago

Previoulsy spinedb_api would happily fall back to 'Base' alternative but now I get an exception

Did it? I'm not sure if that would be correct?

soininen commented 2 years ago

Did it? I'm not sure if that would be correct?

Sorry, forget about that comment. I was confused of what was going on here.

manuelma commented 2 years ago

Might work better in latest master. Unfortunately I forgot to pull before pushing so now we have a merge commit. Please let me know if there are still issues.

manuelma commented 2 years ago

I also added a couple of tests.

soininen commented 2 years ago

Great! The value update tests in Flextool3 interface now pass. Other tests fail, though, need to investigate them next... I think we are done here, though. Thanks a lot, @manuelma!