-
Notifications
You must be signed in to change notification settings - Fork 917
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(parser structure): Refactor IS parser #6262
Conversation
PR Analysis
PR Feedback
How to useInstructions
|
def fetch_production( | ||
zone_key: ZoneKey = ZoneKey("IS"), | ||
session: Session | None = None, | ||
target_datetime: datetime | None = None, | ||
logger: Logger = getLogger(__name__), | ||
) -> list[dict]: | ||
"""Requests the last known production mix (in MW) of a given country.""" | ||
r = session or Session() |
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.
Suggestion: It's better to separate the creation of the Session object from the fetch_production function. This way, the same Session can be reused across multiple requests, which can improve performance by reusing the underlying TCP connection.
def fetch_production( | |
zone_key: ZoneKey = ZoneKey("IS"), | |
session: Session | None = None, | |
target_datetime: datetime | None = None, | |
logger: Logger = getLogger(__name__), | |
) -> list[dict]: | |
"""Requests the last known production mix (in MW) of a given country.""" | |
r = session or Session() | |
def fetch_production( | |
zone_key: ZoneKey = ZoneKey("IS"), | |
session: Session, | |
target_datetime: datetime | None = None, | |
logger: Logger = getLogger(__name__), | |
) -> list[dict]: | |
"""Requests the last known production mix (in MW) of a given country.""" |
obj = res.json() | ||
mix = ProductionMix( | ||
geothermal=obj["geothermal"], | ||
hydro=obj["hydro"], | ||
oil=obj["oil"], | ||
) |
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.
Suggestion: It's a good practice to check if the response from the server is in the expected format before accessing its content. This can prevent unexpected errors.
obj = res.json() | |
mix = ProductionMix( | |
geothermal=obj["geothermal"], | |
hydro=obj["hydro"], | |
oil=obj["oil"], | |
) | |
obj = res.json() | |
if not all(key in obj for key in ["geothermal", "hydro", "oil"]): | |
raise ParserException( | |
parser="amper_landsnet.py", | |
message=f"Response from {SOURCE} is missing expected keys", | |
zone_key=zone_key, | |
) | |
mix = ProductionMix( | |
geothermal=obj["geothermal"], | |
hydro=obj["hydro"], | |
oil=obj["oil"], | |
) |
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 think the endpoint is configured to only accept our production server IP and IPs in Iceland. So might be hard to get a a sample.
The changes look good though!
Ah ok that might explain some stuff! I will try to get a sample through a proxy |
Description
Refactoring the IS parser to use the new datastructure.
Preview
Testing is currently pending because the website seems to be down so I can't get a test sample.
Double check
poetry run test_parser "zone_key"
pnpx prettier --write .
andpoetry run format
to format my changes.