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.

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

Generalize checking for boolean policy parameter values #1782

Merged
merged 2 commits into from
Dec 20, 2017
Merged

Generalize checking for boolean policy parameter values #1782

merged 2 commits into from
Dec 20, 2017

Conversation

martinholmer
Copy link
Collaborator

This pull request implements a request from @hdoupe in PolicyBrain 782.

@codecov-io
Copy link

codecov-io commented Dec 20, 2017

Codecov Report

Merging #1782 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1782   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files          37      37           
  Lines        3028    3028           
======================================
  Hits         3028    3028
Impacted Files Coverage Δ
taxcalc/policy.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e319125...11fc993. Read the comment docs.

@hdoupe
Copy link
Collaborator

hdoupe commented Dec 20, 2017

Thanks @martinholmer. I think this should work. I'll need to upgrade to Tax-Calculator 0.14.2 before I can test this. I'll have some feed back for you before the end of the day.

@hdoupe
Copy link
Collaborator

hdoupe commented Dec 20, 2017

@martinholmer This resolves the issue described in PolicyBrain #780. I implemented a temporary fix in this commit. This fix will be removed once the next version of Tax-Calculator is released.

The fix isn't necessarily bad, but it does introduce unneeded complexity. Further, I think it makes PolicyBrain a little too opinionated. Past experience suggests that parameter checking and conversion at this level should be left to taxcalc.

Thanks for quickly addressing this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants