Skip to content
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.

Sign up for GitHub

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 warning/error parsing #800

Merged
merged 7 commits into from
Jan 18, 2018
Merged

Refactor warning/error parsing #800

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d0fe0a7
Add failing test for parse_errors_warnings
Jan 11, 2018
04c336d
Change order of parse_errors_warnings return dict
Jan 11, 2018
0578c21
Update error appending methods
Jan 11, 2018
a32dfcc
Move warning/error message appending to separate function
Jan 11, 2018
d2014d5
Remove redundant code
Jan 11, 2018
8ae2b43
Use defaultdict
Jan 11, 2018
4102204
Use defaultdict for fake class
Jan 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 27 additions & 29 deletions webapp/apps/taxbrain/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import os
import tempfile
import re
from collections import defaultdict

#Mock some module for imports because we can't fit them on Heroku slugs
from mock import Mock
Expand Down Expand Up @@ -151,6 +152,21 @@ def normalize(x):
return ans


def append_errors_warnings(errors_warnings, append_func):
"""
Appends warning/error messages to some object, append_obj, according to
the provided function, append_func
"""
for action in ['warnings', 'errors']:
for param in errors_warnings[action]:
for year in sorted(
errors_warnings[action][param].keys(),
key=lambda x: int(x)
):
msg = errors_warnings[action][param][year]
append_func(param, msg)


def parse_errors_warnings(errors_warnings, map_back_to_tb):
"""
Parse error messages so that they can be mapped to Taxbrain param ID. This
Expand All @@ -160,7 +176,7 @@ def parse_errors_warnings(errors_warnings, map_back_to_tb):
returns: dictionary 'parsed' with keys: 'errors' and 'warnings'
parsed['errors/warnings'] = {year: {tb_param_name: 'error message'}}
"""
parsed = {'errors': {}, 'warnings': {}}
parsed = {'errors': defaultdict(dict), 'warnings': defaultdict(dict)}
for action in errors_warnings:
msgs = errors_warnings[action]
if len(msgs) == 0:
Expand All @@ -171,11 +187,9 @@ def parse_errors_warnings(errors_warnings, map_back_to_tb):
msg_spl = msg.split()
msg_action = msg_spl[0]
year = msg_spl[1]
curr_id = msg_spl[2]
curr_id = msg_spl[2][1:]
msg_parse = msg_spl[2:]
if year not in parsed[action]:
parsed[action][year] = {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use a defaultdict here instead, i.e.

from collections import defaultdict  # at the top
...
parsed = {'errors': defaultdict(dict), 'warnings': defaultdict(dict)}

Then you can get rid of the year not in check. This is a common build pattern which is why defaultdict was added.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh nice. defaultdict is pretty nifty. I wish I'd known about defaultdict earlier.

parsed[action][year][curr_id[1:]] = ' '.join([msg_action] + msg_parse +
parsed[action][curr_id][year] = ' '.join([msg_action] + msg_parse +
['for', year])

return parsed
Expand Down Expand Up @@ -467,14 +481,10 @@ def submit_reform(request, user=None, json_reform_id=None):
# only handle GUI errors for now
if ((taxcalc_errors or taxcalc_warnings)
and personal_inputs is not None):
for action in errors_warnings:
for year in sorted(errors_warnings[action].keys(),
key=lambda x: float(x)):
for param in errors_warnings[action][year]:
personal_inputs.add_error(
param,
errors_warnings[action][year][param]
)
append_errors_warnings(
errors_warnings,
lambda param, msg: personal_inputs.add_error(param, msg)
)
has_parse_errors = any(['Unrecognize value' in e[0]
for e in personal_inputs.errors.values()])
if no_inputs:
Expand Down Expand Up @@ -592,22 +602,10 @@ def file_input(request):
if (has_errors and
(errors_warnings['warnings'] or errors_warnings['errors'])):
errors.append(OUT_OF_RANGE_ERROR_MSG)
# group messages by parameter name
group_by_param = {}
for action in errors_warnings:
for year in sorted(errors_warnings[action].keys(),
key=lambda x: float(x)):
for param in errors_warnings[action][year]:
if param in group_by_param:
group_by_param[param].append(
errors_warnings[action][year][param]
)
else:
group_by_param[param] = \
[errors_warnings[action][year][param]]
# append to errors
for param in group_by_param:
errors += group_by_param[param]
append_errors_warnings(
errors_warnings,
lambda _, msg: errors.append(msg)
)
else:
return redirect(unique_url)
else:
Expand Down
52 changes: 51 additions & 1 deletion webapp/apps/test_assets/test_param_formatters.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,11 @@
import pytest
import taxcalc
import numpy as np
from collections import defaultdict

from ..taxbrain.views import (read_json_reform, parse_errors_warnings,
append_errors_warnings)

from ..taxbrain.views import read_json_reform, parse_errors_warnings
from ..taxbrain.helpers import (get_default_policy_param_name, to_json_reform)

from test_reform import (test_coverage_fields, test_coverage_reform,
Expand Down Expand Up @@ -93,6 +96,53 @@ 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"}}
}
errors = []
exp = ["msg1", "msg2", "msg3", "msg4"]

append_errors_warnings(
errors_warnings,
lambda _, msg: errors.append(msg)
)

assert errors == 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 = defaultdict(list)

def add_error(self, param_name, msg):
self.errors[param_name].append(msg)

exp = {"param1": ["msg1", "msg2"],
"param2": ["msg3", "msg4"]}

personal_inputs = FakeForm()
append_errors_warnings(
errors_warnings,
lambda param, msg: personal_inputs.add_error(param, msg)
)

assert (exp["param1"] == personal_inputs.errors["param1"] and
exp["param2"] == personal_inputs.errors["param2"])


###############################################################################
# Test read_json_reform
# 3 Cases:
Expand Down
50 changes: 27 additions & 23 deletions webapp/apps/test_assets/test_reform.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,30 +372,34 @@

exp_errors_warnings = {
'errors': {
'2017': {
'FICA_ss_trt': 'ERROR: _FICA_ss_trt value -1.0 < min value 0 for 2017',
'II_brk4_0': 'ERROR: _II_brk4_0 value 500.0 < min value 91900.0 for _II_brk3_0 for 2017'},
'2018': {
'FICA_ss_trt': 'ERROR: _FICA_ss_trt value -1.0 < min value 0 for 2018',
'II_brk4_0': 'ERROR: _II_brk4_0 value 511.25 < min value 93967.75 for _II_brk3_0 for 2018'},
'2019': {'II_brk4_0': 'ERROR: _II_brk4_0 value 522.7 < min value 96072.63 for _II_brk3_0 for 2019'},
'2020': {'II_brk4_0': 'ERROR: _II_brk4_0 value 534.77 < min value 98291.91 for _II_brk3_0 for 2020'},
'2021': {'II_brk4_0': 'ERROR: _II_brk4_0 value 547.5 < min value 100631.26 for _II_brk3_0 for 2021'},
'2022': {'II_brk4_0': 'ERROR: _II_brk4_0 value 560.8 < min value 103076.6 for _II_brk3_0 for 2022'},
'2023': {'II_brk4_0': 'ERROR: _II_brk4_0 value 574.09 < min value 105519.52 for _II_brk3_0 for 2023'},
'2024': {'II_brk4_0': 'ERROR: _II_brk4_0 value 587.87 < min value 108051.99 for _II_brk3_0 for 2024'},
'2025': {'II_brk4_0': 'ERROR: _II_brk4_0 value 601.86 < min value 110623.63 for _II_brk3_0 for 2025'},
'2026': {'II_brk4_0': 'ERROR: _II_brk4_0 value 616.18 < min value 113256.47 for _II_brk3_0 for 2026'},
'2027': {'II_brk4_0': 'ERROR: _II_brk4_0 value 630.97 < min value 115974.63 for _II_brk3_0 for 2027'}
'FICA_ss_trt': {
'2017': 'ERROR: _FICA_ss_trt value -1.0 < min value 0 for 2017',
'2018': 'ERROR: _FICA_ss_trt value -1.0 < min value 0 for 2018'
},
'II_brk4_0': {
'2017': 'ERROR: _II_brk4_0 value 500.0 < min value 91900.0 for _II_brk3_0 for 2017',
'2018': 'ERROR: _II_brk4_0 value 511.25 < min value 93967.75 for _II_brk3_0 for 2018',
'2019': 'ERROR: _II_brk4_0 value 522.7 < min value 96072.63 for _II_brk3_0 for 2019',
'2020': 'ERROR: _II_brk4_0 value 534.77 < min value 98291.91 for _II_brk3_0 for 2020',
'2021': 'ERROR: _II_brk4_0 value 547.5 < min value 100631.26 for _II_brk3_0 for 2021',
'2022': 'ERROR: _II_brk4_0 value 560.8 < min value 103076.6 for _II_brk3_0 for 2022',
'2023': 'ERROR: _II_brk4_0 value 574.09 < min value 105519.52 for _II_brk3_0 for 2023',
'2024': 'ERROR: _II_brk4_0 value 587.87 < min value 108051.99 for _II_brk3_0 for 2024',
'2025': 'ERROR: _II_brk4_0 value 601.86 < min value 110623.63 for _II_brk3_0 for 2025',
'2026': 'ERROR: _II_brk4_0 value 616.18 < min value 113256.47 for _II_brk3_0 for 2026',
'2027': 'ERROR: _II_brk4_0 value 630.97 < min value 115974.63 for _II_brk3_0 for 2027'
}
},
'warnings': {
'2020': {'STD_3': 'WARNING: _STD_3 value 150.0 < min value 9900.32 for 2020'},
'2021': {'STD_3': 'WARNING: _STD_3 value 153.57 < min value 10138.33 for 2021'},
'2022': {'STD_3': 'WARNING: _STD_3 value 157.3 < min value 10387.12 for 2022'},
'2023': {'STD_3': 'WARNING: _STD_3 value 161.03 < min value 10635.66 for 2023'},
'2024': {'STD_3': 'WARNING: _STD_3 value 164.89 < min value 10893.32 for 2024'},
'2025': {'STD_3': 'WARNING: _STD_3 value 168.81 < min value 11154.96 for 2025'},
'2026': {'STD_3': 'WARNING: _STD_3 value 172.83 < min value 11422.83 for 2026'},
'2027': {'STD_3': 'WARNING: _STD_3 value 176.98 < min value 11699.38 for 2027'}
'STD_3': {
'2020': 'WARNING: _STD_3 value 150.0 < min value 9900.32 for 2020',
'2021': 'WARNING: _STD_3 value 153.57 < min value 10138.33 for 2021',
'2022': 'WARNING: _STD_3 value 157.3 < min value 10387.12 for 2022',
'2023': 'WARNING: _STD_3 value 161.03 < min value 10635.66 for 2023',
'2024': 'WARNING: _STD_3 value 164.89 < min value 10893.32 for 2024',
'2025': 'WARNING: _STD_3 value 168.81 < min value 11154.96 for 2025',
'2026': 'WARNING: _STD_3 value 172.83 < min value 11422.83 for 2026',
'2027': 'WARNING: _STD_3 value 176.98 < min value 11699.38 for 2027'
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One software engineering practice you might want to consider is separating "code" from "data". In this case the "data" is these warnings, which could be saved as like a separate JSON file somewhere and loaded into Python (using the default import json library). The main goal is to abstract away this kind stuff from the core code logic, which makes your code much more readable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I see. I'll open a follow up PR to clean up test_reform.py. There are plenty of data there that need to be moved into JSON files.

}
}