Refactor warning/error parsing

[hdoupe]

This PR refactors the warning/error parsing logic. This reduces duplicated code and puts the warning/error parsing logic under test.

cc @codekansas

[codekansas]

It's very late here so don't take my comments seriously. I just added some pointers here. I should go to bed.

[codekansas]

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.

[hdoupe]

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

[codekansas]

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_warnings be a method with two arguments, msg and param

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

# line 500
append_errors_warnings(
    errors_warnings,
    personal_inputs,
    lambda msg, param: personal_inputs.add_error(param, msg))

Then the append_errors_warnings function would be

def append_errors_warnings(errors_warnings, append_obj, append_func):
    ...
    append_func(msg, param)

Admittedly lambdas are frowned upon by some people but I think it makes for less redundant code here.

[hdoupe]

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?

[codekansas]

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,
   ...
)

[hdoupe]

I hadn't thought about preserving "blame" before. That makes a lot of sense. Thanks for the pointer.

[codekansas]

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)

[codekansas]

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.

[hdoupe]

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.

[hdoupe]

@codekansas Thanks for the review. Let me know what you think of the changes.

[codekansas]

@hdoupe LGTM

[hdoupe]

Thanks for the review @codekansas