-
Notifications
You must be signed in to change notification settings - Fork 55
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
patch(meters.py): handle mistyping #2211
Conversation
@@ -41,6 +41,7 @@ def __init__(self, property_id, org_id, excluded_meter_ids, scenario_ids=None): | |||
self._cache_factors = None | |||
self._cache_org_country = None | |||
|
|||
scenario_ids = scenario_ids if scenario_ids is not None else [] |
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 we could also set the default for scenario_ids
as []
when defining the parameters.
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 used this pattern because python's default parameters are only evaluated once when the function is defined, so dicts and lists can cause some surprising results if modified. Check this out here: https://docs.python-guide.org/writing/gotchas/#mutable-default-arguments
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.
Ah ok - didn't realize this could have been an issue! 👍
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.
Looks good! I tested the property detail meter page works with and without scenario Meters.
@@ -41,6 +41,7 @@ def __init__(self, property_id, org_id, excluded_meter_ids, scenario_ids=None): | |||
self._cache_factors = None | |||
self._cache_org_country = None | |||
|
|||
scenario_ids = scenario_ids if scenario_ids is not None else [] |
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.
Ah ok - didn't realize this could have been an issue! 👍
Small patch for mistyped query parameter. This isn't a current issue (ie it should not actually cause runtime exceptions) but is an error nonetheless.