Refactor warning/error parsing #800
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,7 +3,10 @@ | |
| import taxcalc | ||
| import numpy as np | ||
|
|
||
| from ..taxbrain.views import (read_json_reform, parse_errors_warnings, | ||
|
There was a problem hiding this comment. One handy software engineering tool you might be interested in is "blame". Basically, it's a tool that shows the last person to make changes to each line of code. GitHub has a more thorough explanation here. This isn't a review comment per se, but one thing to keep in mind when using that tool is writing code in a way that "preserves blame", in other words, so that you don't make changes to lines that you don't edit. In this case, the best way to "preserve blame" would be to write each import on a new line, so that if you add your own import, it doesn't update the blame on someone else's import, so you know the last person who thought that import was important to change. i.e. from ..taxbrain.views import (
read_json_reform,
parse_errors_warnings,
...
)There was a problem hiding this comment. I hadn't thought about preserving "blame" before. That makes a lot of sense. Thanks for the pointer. There was a problem hiding this comment. Check out the blame tool, it is useful for figuring out who wrote something if you're trying to figure out why it was done that way (i.e. poorly documented code) |
||
| append_errors_warnings, append_ew_file_input, | ||
| append_ew_personal_inputs) | ||
|
|
||
| from ..taxbrain.helpers import (get_default_policy_param_name, to_json_reform) | ||
|
|
||
| from test_reform import (test_coverage_fields, test_coverage_reform, | ||
|
|
@@ -93,6 +96,51 @@ def test_parse_errors_warnings(): | |
| np.testing.assert_equal(act, exp_errors_warnings) | ||
|
|
||
|
|
||
| def test_append_ew_file_input(): | ||
| """ | ||
| Tests append_errors_warnings when add warning/error messages from the | ||
| file input interface | ||
| """ | ||
| errors_warnings = {"warnings": {"param1": {"2017": "msg2", "2016": "msg1"}}, | ||
| "errors": {"param2": {"2018": "msg3", "2019": "msg4"}} | ||
| } | ||
| error_list = [] | ||
| exp = ["msg1", "msg2", "msg3", "msg4"] | ||
|
|
||
| append_errors_warnings(errors_warnings, error_list, append_ew_file_input) | ||
|
|
||
| assert error_list == exp | ||
|
|
||
| def test_append_ew_personal_inputs(): | ||
| """ | ||
| Tests append_errors_warnings when adding warning/error messages from the | ||
| GUI input interface | ||
| """ | ||
| errors_warnings = {"warnings": {"param1": {"2017": "msg2", "2016": "msg1"}}, | ||
| "errors": {"param2": {"2018": "msg3", "2019": "msg4"}} | ||
| } | ||
| # fake PersonalExemptionForm object to simulate add_error method | ||
| class FakeForm: | ||
| def __init__(self): | ||
| self.errors = {} | ||
|
|
||
| def add_error(self, param_name, msg): | ||
| if param_name in self.errors: | ||
| self.errors[param_name].append(msg) | ||
| else: | ||
| self.errors[param_name] = [msg] | ||
|
|
||
| exp = {"param1": ["msg1", "msg2"], | ||
| "param2": ["msg3", "msg4"]} | ||
|
|
||
| fake_form = FakeForm() | ||
| append_errors_warnings(errors_warnings, fake_form, | ||
| append_ew_personal_inputs) | ||
|
|
||
| assert (exp["param1"] == fake_form.errors["param1"] and | ||
| exp["param2"] == fake_form.errors["param2"]) | ||
|
|
||
|
|
||
| ############################################################################### | ||
| # Test read_json_reform | ||
| # 3 Cases: | ||
|
|
||
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 seems redundant, since you only use this function once... Probably a cleaner way to do this would be to make the third argument of
append_errors_warningsbe a method with two arguments,msgandparamThen the
append_errors_warningsfunction would beAdmittedly lambdas are frowned upon by some people but I think it makes for less redundant code here.
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 don't have any aversion to lambdas. I think this looks a little cleaner.
Out of curiosity, why do some people frown upon lambda functions?