-
Notifications
You must be signed in to change notification settings - Fork 947
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
1899/czech parser #5112
1899/czech parser #5112
Conversation
✅ Deploy Preview for phenomenal-syrniki-c5ceaa ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some early feedback before a full review.
It looks like we can replace arrow with just native datetime and timedelta together with the datetime.isoformat()
function for formatting it. That should help geting rid of some of the type errors.
Some of the functions here seem to be a little more complex than they need to be. But I'll take a look at that tomorrow when I do a full review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything is working but there are a few improvemnts that need to be made here and I have added suggestions for most.
I'll re-review this when the changes have been addressed. If there are any questions don't hesitate to ping me or reach out on slack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is making some nice progress so good work!
I have some more suggestions but after that I think this is more or less ready to be merged. 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks great! Thanks for implementing this!
Just one final question to double check something then we can merge this.
Does is the datetime forward looking or backwards looking?
I.e. does 15:00 mean the production between 15:00 and 15:14:59 or 14:45:01 and 15:00?
Edit:
Oh and if you wish please do add yourself to the list of contributors in the CZ.yaml file.
DateTime is backward looking as you need to provide fromDate and TargetDate to obtain results. The result is in the range of these two parameters. I've added my GitHub nickname among contributors in the CZ.yaml file. |
Okay then we need to adjust these datetimes to be forward looking as that is what the backend expects. PS: It don't look like you pushed the changes with your nickname to this branch yet. |
Hi Viktor, sorry for the delay. Yes, please give me some hints on how to do it as I have no idea now. I guess you are right I haven't pushed it yet |
Okay so I have dug into the code a bit and it does look like it's returning the correct format so that's great! I did find a place where we can improve things though, if you add the following function we can both improve things and reduce code duplication. Here is the function we need to add: def get_target_datetime(dt: Optional[datetime]) -> datetime:
if dt is None:
now = datetime.now()
dt = (now - timedelta(minutes=now.minute % 15)).replace(second=0, microsecond=0)
return dt This function gets the nearest 15 minute step instead of the full hour like before and can be re-used across the functions. And here is what we need to replace: - if not target_datetime:
- target_datetime = datetime.now().replace(minute=0, second=0, microsecond=0)
+ target_datetime = get_target_datetime(target_datetime) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a bit more feedback than necessary to get this merged since you requested it but it's only the last comment (about the exchangeForecasts) that need to be resolved before we can merge this.
…de exchange forecast
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🎉
Let's get this merged! Thanks for taking the time to create this parser and bearing with me in making the requested changes.
For some reason the CI is failing (I don't think it should) but I don't have time to look into that more right now but I'll try and figure out why it's failing this weekend. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🎉
Now that the new parser is live, I noticed that pumping data for pumped hydro is no longer available; IIRC it was available from the ENTSOE data source. It looks like CEPS does provide this data, so it should be possible to restore this functionality. |
Issue
Description
replace ENTSOE data provider with the official Czech grid maintainer CEPS
Preview
Double check
poetry run test_parser "zone_key"
pnpx prettier --write .
andpoetry run format
to format my changes.