Switch @validate_country to decorator #1970
Conversation
ldorner
force-pushed
the
ldorner/use_validation_hook_check_country
branch
from
f4ea3f7 to
e277100
Compare
| @@ -50,9 +51,6 @@ def get_economic_impact( | |||
| dict: The economic impact. | |||
| """ | |||
| print(f"Got request for {country_id} {policy_id} {baseline_policy_id}") | |||
There was a problem hiding this comment.
With this change, this line will no longer print for invalid countries. Is it important to preserve this behavior (ie: we want to print all requested countries)? If so, perhaps it's best to add this printing/logging elsewhere. Otherwise, perhaps it can be removed entirely?
There was a problem hiding this comment.
No, I don't think it's important to maintain this logging. Removing is totally fine.
There was a problem hiding this comment.
It looks like this function moved to policyengine_api/routes/economy_routes.py. I've made the update there and removed his comment.
|
Thank you for this work @ldorner. I will review probably tomorrow, depending on progress on some things I need to catch up on from some time off. |
ldorner
force-pushed
the
ldorner/use_validation_hook_check_country
branch
from
b897af6 to
6030263
Compare
|
I rebased to pull in recent changes/resolve conflicts. |
|
@ldorner Unfortunately, it looks like the added test still fails due to accessing the |
Sorry! Bad merge. Fixed now :) |
|
@ldorner Thanks again for this, will review early tomorrow |
Overview
Currently, there is a repeated check and short-circuit in many functions to validate existence of the given country ID. This PR refactors
validate_countryas a decorator to DRY the code. We may consider transitioning this to abefore_requesthook to further reduce code repetition.This is essentially a refactor. There should be no functional change.
Changes
validate_countryto change it into a decorator/wrappervalidate_country()check & short circuit ->@validate_countrydecoratorvalidate_countryfunctiontest_policy.pyto ensure that the decorator functionality works