-
Notifications
You must be signed in to change notification settings - Fork 931
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
refactor: upgrade CA-SK with event classes #6013
refactor: upgrade CA-SK with event classes #6013
Conversation
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.
Great start!
Thanks for doing this, a few things to consider before we can merge this though.
parsers/CA_SK.py
Outdated
target_datetime: datetime | None, | ||
url: str, | ||
zone_key: ZoneKey, | ||
) -> typing.Any: |
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.
Since we are returning json the type should be list | dict | None
, which makes the type checking a little bit better.
It's possible we can even omit None.
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 didn't want to restrict the return types because a JSON payload could feasibly be a bool
, float
, int
, or str
as well. In fact, while the production URL returns a dict
, judging from the existing parser, the consumption source seems to return a str
(it's hard to be sure while it's down). How about this?
) -> typing.Any: | |
) -> dict | str: |
130948b
to
c20e9c1
Compare
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 finishing touches are needed but beyond these it looks good!
1f2394a
to
37bc7d6
Compare
Upgrade the parser to use the ProductionBreakdown, ProductionMix, TotalConsumption, and ZoneKey classes. This should yield no functional change. Note that this does not resolve the current issue with the fetch_consumption function. Additionally: - Comply with Black's 88-column default line length limit. - Factor out duplicate code. Refs: electricitymaps#5986, electricitymaps#6011, electricitymaps#6013
- Be more specific about the return type of the _request function. - Import the Any type directly. - Swap out pytz in exchange for zoneinfo. - Use the ProductionMix.add_value method instead of a dictionary comprehension. Refs: electricitymaps#5986, electricitymaps#6011, electricitymaps#6013
Swap out TotalConsumption for TotalConsumptionList, as the former is considered an implementation detail while the latter is part of the public API. Refs: electricitymaps#5986, electricitymaps#6011, electricitymaps#6013
37bc7d6
to
4a60f16
Compare
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! 🎉
Issue
#5986, #6011
Description
Upgrade the parser to use the
ProductionBreakdown
,ProductionMix
,TotalConsumption
, andZoneKey
classes. This should yield no functional change. Note that this does not resolve the current issue with thefetch_consumption
function. Additionally:Double check
poetry run test_parser "zone_key"
pnpx prettier --write .
andpoetry run format
to format my changes.