Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Do not init facade in adapter #47

Merged
merged 9 commits into from
Oct 6, 2023
Merged

Do not init facade in adapter #47

merged 9 commits into from
Oct 6, 2023

Conversation

henhuy
Copy link
Contributor

@henhuy henhuy commented Aug 16, 2023

Closes #46

@henhuy henhuy marked this pull request as draft August 16, 2023 10:06
@henhuy
Copy link
Contributor Author

henhuy commented Aug 16, 2023

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
Copy link
Contributor Author

henhuy commented Aug 16, 2023

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...

@henhuy henhuy force-pushed the feature/do_not_init_facade_in_adapter branch from 635c79d to 22be101 Compare August 16, 2023 10:10
FelixMau
FelixMau approved these changes Aug 22, 2023
View reviewed changes
data_adapter_oemof/adapters.py Outdated
@classmethod
def get_default_parameters(cls, struct: dict, mapper: Mapper) -> dict:
def as_dict(self) -> dict:
return self.facade_dict
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you want to keep the as_dict attribute?
To me this appears a little confusing and like an extra step to just change the name.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only synthetic sugar. but i will remove it

data_adapter_oemof/adapters.py
"""
Dispatchable Adapter
"""

type = "dispatchable"
facade = facades.Dispatchable
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have had this Version with facade as DataAdapter attribute before but I do not remember why we changed that.

@FelixMau
Copy link
Contributor

FelixMau commented Aug 28, 2023 •
edited
Loading

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
Copy link
Contributor

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

@henhuy
Copy link
Contributor Author

henhuy commented Sep 28, 2023

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

@FelixMau FelixMau self-assigned this Oct 2, 2023
@FelixMau FelixMau marked this pull request as ready for review October 2, 2023 12:18
@FelixMau
Copy link
Contributor

FelixMau commented Oct 2, 2023

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
Copy link
Contributor Author

henhuy commented Oct 4, 2023

I will have a look today!

henhuy and others added 8 commits October 6, 2023 10:38
@henhuy
Add empty collections folder to tests
f5043b4
@henhuy
Refactor Adapter to not init facade
6e07bf2
@nailend @henhuy
Update lock file [data-adapter]
4ea35fe
@FelixMau @henhuy
Update PROCESS_TYPE_MAP.yaml
aada73e 
No data_adapter test since it should be tested in data_adapter itself
@FelixMau @henhuy
Dataclass dependencies
408e419 
To create a Mapper instance a adapter is neccessary but to create an adapter instance a mapper is neccessary as well. Therefore 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)
@henhuy
Fix test mapping with sequence
23e1c3c
@henhuy
Minor naming change
49971f3
@henhuy
Minor change
f28a453
@henhuy
Copy link
Contributor Author

henhuy commented Oct 6, 2023

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 henhuy force-pushed the feature/do_not_init_facade_in_adapter branch from a261a94 to f28a453 Compare October 6, 2023 08:53
@henhuy
Copy link
Contributor Author

henhuy commented Oct 6, 2023 •
edited
Loading

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).

@henhuy henhuy merged commit c021c23 into dev Oct 6, 2023
1 of 3 checks passed
@henhuy henhuy deleted the feature/do_not_init_facade_in_adapter branch October 6, 2023 11:05
@FelixMau FelixMau mentioned this pull request Oct 13, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FelixMau FelixMau FelixMau approved these changes

@nailend nailend Awaiting requested review from nailend

Assignees

@FelixMau FelixMau

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Do not init facade in adapter
3 participants
@henhuy @FelixMau @nailend