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.

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

Summarize attributes into a request object #8

Closed
meteoDaniel opened this issue Sep 11, 2019 · 5 comments · Fixed by #57
Closed

Summarize attributes into a request object #8

meteoDaniel opened this issue Sep 11, 2019 · 5 comments · Fixed by #57
Assignees
Labels
enhancement New feature or request

Comments

@meteoDaniel
Copy link
Contributor

Several times var(parameter) res (time_resolution) and per(period_type) are parsed to functions. It would make sense to create a dataclass holding these attributes. Within this way we can guarantee all required information are correctly parsed to the different functions.

@meteoDaniel meteoDaniel added the enhancement New feature or request label Sep 11, 2019
@meteoDaniel meteoDaniel self-assigned this Sep 11, 2019
@amotl
Copy link
Member

amotl commented Jun 10, 2020

Dear Daniel,

while working on the cli.py module mentioned within #50, I started to familiarize myself with the code base. There, I just found DWDStationRequest in dwd_station_request.py.

Am I right to assume that this already bundles all request attributes into this very class/object with the intention outlined in your original post?

However, after scanning the code base further, I found DWDStationRequest isn't really used besides some sanity checks for bogus input values within test_dwd_station_request.py.

Did I miss something here or have you just been able to lay the groundwork for upcoming features where DWDStationRequest will be used within some pipeline program as outlined within #21?

With kind regards,
Andreas.

@amotl
Copy link
Member

amotl commented Jun 10, 2020

Dear Benjamin,

now I am seeing your improvements on the stationdata-request-definition branch [1]. Especially, be10b1b will bring in all the meat of the future/advanced canonical collect_data routine, right?

With kind regards,
Andreas.

[1] https://github.com/gutzbenj/python_dwd/commits/Feature/21/stationdata-request-definition

@gutzbenj
Copy link
Member

Dear @amotl ,

it took me quite a while to outline the functioning of DWDStationRequest, but you're absolutely right, the next PR from my side will include a method to source all the data as defined by the request.

@amotl
Copy link
Member

amotl commented Jun 10, 2020

Hi Benjamin,

Right, the next PR from my side will include a method to source all the data as defined by the request.

Thanks, I am absolutely looking forward to that. Right now, my basic cli.py implementation will be based on the current enable_string_parsing branch but it should really be made work on top of stationdata-request-definition.

As far as I can see, #52 can be abandoned then as data_collection.py will vanish altogether. May I humbly ask if you see any time frame for merging stationdata-request-definition into master?

Keep up the great work and with kind regards,
Andreas.

@gutzbenj
Copy link
Member

It shouldn't take more then a few days - I usually want to rely on @meteoDaniel 's feedback/review as a maintainer for any PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants