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

Improvements on scenario.py get_survey_scenario.py #158

Merged
merged 8 commits into from
Feb 27, 2019

Conversation

bonjourmauko
Copy link
Member

@bonjourmauko bonjourmauko commented Feb 26, 2019

Changes

  • Split unit and integration tests
  • Add doc to scenario.py and get_survey_scenario.py
  • Add tests to get_survey_scenario.py
  • Use multipledispatch to reduce code complexity
Split unit and integration tests

The idea of this separation is to have, in one hand, integration tests like calculating income tax.

And, on the other, tests that only test certain functions in isolation.

This is practical as:

  • It helps catching bugs
  • Unit tests are faster
  • We can leave integration tests to test the important things from a metier point of view
Use multipledispatch to reduce code complexity

This module contains a lot of conditional branching, and that's bug prone and makes difficult to encapsulate functionality.

For example, if we want to add conditions by millésime, we end up with a lot of ifs that render the code extremely hard to maintain.

With dynamic dispatch, we can do so in a cleaner way.

This is closely similar to OpenFisca's formulas, although simpler, generic, and more performant.

@bonjourmauko bonjourmauko force-pushed the update-erfs-fpr-doc branch 2 times, most recently from 80dc50e to 057a98e Compare February 27, 2019 08:30
@bonjourmauko bonjourmauko changed the title [WIP] Update ERFS-FPR doc Improvements on scenario.py get_survey_scenario.py Feb 27, 2019
@bonjourmauko bonjourmauko requested review from a team February 27, 2019 08:36
survey_scenario.init_from_data(
data = data,
rebuild_input_data = rebuild_input_data,
)
return survey_scenario


@dispatch(TaxBenefitSystem, Reform) # type: ignore
Copy link
Member

Choose a reason for hiding this comment

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

Id do not know this decorator and it is not straightforward to understand what it does. Could you provide some ref.

Copy link
Member Author

@bonjourmauko bonjourmauko Feb 27, 2019

Choose a reason for hiding this comment

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

There is a lot of complexity checking whether a value is present or None, or if it has a certain type, and that implies a lot of if and is bug prone.

What this does is to validate data : depending on the value of the argument used, it'll dispatch to a version of the function that handles that specific case.

What we do is to be sure to dispatch to the expected behaviour by type checking the arguments.

Usually you don't need this, but I think for this repo it'll help us catch several bugs and reduce code complexity with little change.

We'll be able to factor out several branch conditions without risk.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can add a comment in the code for other readers as well if that helps.

@bonjourmauko bonjourmauko merged commit 787343d into release/1.0.0 Feb 27, 2019
@bonjourmauko bonjourmauko deleted the update-erfs-fpr-doc branch February 27, 2019 11:17
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.

2 participants