Features/#59 move resource management to importlib resources#119
Features/#59 move resource management to importlib resources#119adelmemariani wants to merge 7 commits intodevfrom
Conversation
| import egon.data.processing.openstreetmap as process_osm | ||
| import egon.data.importing.zensus as import_zs | ||
|
|
||
| from importlib_resources import files |
There was a problem hiding this comment.
This is a third party library import. According to "CONTRIBUTING.rst" this should be placed in a section between builtin imports and package local imports. But I see that others messed up the import order before you already.
Also, the importlib-resources package should be added to the depen dencies in "setup.py".
| print('*-*-*-*-*-*') | ||
| print(os.path.abspath(os.path.join(os.path.dirname( | ||
| __file__ ), '..', '..', 'processing', 'vg250'))) | ||
| print(files('egon.data.processing').joinpath('vg250')) |
There was a problem hiding this comment.
Since you removed those changes in a later commit, I think you committed them by accident and know that such debug statements usually shouldn't be part of published commits. But now that I see how you used those statements, let me point out, that you could also have tried them out in an interactive console session, obviating the need to write them into a file in the first place.
| def main(context, **kwargs): | ||
| os.environ["AIRFLOW_HOME"] = os.path.dirname(egon.data.airflow.__file__) | ||
| #os.environ["AIRFLOW_HOME"] = os.path.dirname(egon.data.airflow.__file__) | ||
| os.environ["AIRFLOW_HOME"] = os.path.dirname(files(egon.data.airflow).joinpath('__init__.py')) |
There was a problem hiding this comment.
This line exceeds the 79 character limit. Also os.path.dirname just removes the '__init__.py' component, so files(egon.data.airflow) should suffice.
There was a problem hiding this comment.
Using the files(egon.data.airflow) yields TypeError: str expected, not PosixPath. So, I changed it to str(files(egon.data.airflow)). I hope adding str() is ok.
| os.path.dirname(__file__), osm_config["target"]["path"] | ||
| #os.path.dirname(__file__), | ||
| files(import_openstreetmap), | ||
| osm_config["target"]["path"] |
There was a problem hiding this comment.
There's no reason to split up the original line. Our automatic code formatting tool will join them again anyway.
| os.path.dirname(__file__), osm_config["target"]["path"] | ||
| #os.path.dirname(__file__), | ||
| files(import_openstreetmap), | ||
| osm_config["target"]["path"] |
There was a problem hiding this comment.
Will also be joined when formatting code.
| # Read database configuration from docker-compose.yml | ||
| package_path = egon.data.__path__[0] | ||
| #package_path = egon.data.__path__[0] | ||
| package_path = files('egon.data') |
There was a problem hiding this comment.
When the package has been imported, you can also just write files(egon.data), guarding against typos.
|
|
||
| from importlib_resources import files | ||
|
|
||
|
|
There was a problem hiding this comment.
This style change is correct, but should be done in a separate commit. Ideally, style changes should be separate from changes modifying behaviour.
gnn
left a comment
gnn
left a comment
There was a problem hiding this comment.
Please remove the commented code and fix the minor style infringements.
gnn
left a comment
gnn
left a comment
There was a problem hiding this comment.
Ideally these changes would be split up into two commits, with separate explanatory commit messages.
| #os.path.abspath(os.path.join(os.path.dirname( | ||
| # __file__ ), '..', '..', 'processing', 'vg250')) | ||
| files('egon.data.processing').joinpath('vg250') | ||
|
|
There was a problem hiding this comment.
I think files('egon.data.processing.vg250') should do the same.
…buses Fixes/#119 isolated ch4 foreign buses
2 participants
It is intended to address the issues#59 'Move resource management to
importlib.resources'