Do not init facade in adapter

sedos-project / data_adapter_oemof

This respository holds the data adapters to connect oemof with the OEDatamodel-concrete.

GNU Affero General Public License v3.0

0 stars 1 forks source link

Do not init facade in adapter #47

Closed henhuy closed 9 months ago

henhuy commented 10 months ago

Closes #46

henhuy commented 10 months ago

For now I only added a simple test to check if my new solution works. Other tests and implementations are not adopted and will fail

henhuy commented 10 months ago

Now Adapter class looks very empty and could almost be removed (mapper inside Adapter does most parts). Maybe next time we should refactor even more...

FelixMau commented 10 months ago

Merged the "fix tests" updates from dev into here for easier maintenance and development:

Todos:

[ ] New Adapter class has no attribute "name" anymore (see mapping tests)
[ ] New Adapter class has no "parametrize_dataclass" anymore ->replace by init the Adapter
[ ] Resolve "as_dict" comment from above

FelixMau commented 9 months ago

@henhuy Should I have a look into fixing the tests here or do you want to continue working on this issue?

henhuy commented 9 months ago

Hey @FelixMau, would be nice if you could continue - as I'm deep into solph code, trying to implement TSAM... thx in advance!

FelixMau commented 9 months ago

To create a Mapper instance a adapter is neccessary but to create an adapter instance a mapper is neccessary as well since we wanted to change __init__ behaviour. This circular behavior might be problematic and is not good style I believe, we should discuss solutions if there is capacity.

To solve this issue a new function has been created in Mapper to do the job the adapter instance did before (giving out all names and types of occouring fields)

Tests are still failing due to missing changes from dev. Should I merge dev into here?

henhuy commented 9 months ago

I will have a look today!

henhuy commented 9 months ago

To create a Mapper instance a adapter is neccessary but to create an adapter instance a mapper is neccessary as well since we wanted to change __init__ behaviour. This circular behavior might be problematic and is not good style I believe, we should discuss solutions if there is capacity.

For Mapper instance only the class of Adapter has to be given (not an instance) - so IMO this is not a problem. (If instance would have been needed, you are right this would cause cyclic problems!)

To solve this issue a new function has been created in Mapper to do the job the adapter instance did before (giving out all names and types of occouring fields)

Looks nice - good idea! - I only renamed some names regarding your new namedtuple.

Tests are still failing due to missing changes from dev. Should I merge dev into here?

I rebased dev into this branch.

henhuy commented 9 months ago

By the way - nice job of joining year-related processes! Hope we can merge this soon. We are coming closer to our goal of building datapackage from databus data, I think.

We have to adapt "goal" CSVs, as non-given parameters are not set with default value from facade anymore by the adapter class (as they aren't necessary for setting up facade).