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

Internationalize scenarios #444

Merged
merged 10 commits into from
Feb 16, 2017
Merged

Internationalize scenarios #444

merged 10 commits into from
Feb 16, 2017

Conversation

fpagnoux
Copy link
Member

@fpagnoux fpagnoux commented Feb 2, 2017

Connected to openfisca/openfisca-web-api#76

A lot of the logic to build an openfisca scenario from a json is duplicated in all countries.

This is mainly used to serve the tax and benefits system with web-api, but it has other applications.

Inside this duplicated code, we can find... a lot of duplication, as the entities are usually hardcoded, and similar logic is repeated for Familles, FoyerFiscal...

This PR generalise a big chunk of the code, and moves it from france to core.

To be honest, yes, this is still ugly and full of debt. The scenarios need a huge refactoring, which would probably be done the best by throwing everything away.

However, we have no visibility about when this refactoring will come, and having a turnkey working api will be useful for tunisia.

Copy link
Member

@benjello benjello left a comment

Choose a reason for hiding this comment

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

Je ne suis pas sûr de tout maîtriser, mais avançons !

@fpagnoux fpagnoux force-pushed the internationalize-scenarios branch from 2d14927 to 131dd42 Compare February 4, 2017 22:03
@cbenz cbenz self-assigned this Feb 6, 2017
Copy link
Member

@cbenz cbenz left a comment

Choose a reason for hiding this comment

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

Small issue with naming

@@ -0,0 +1,150 @@
# -*- coding: utf-8 -*-
Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly, you chose to extract biryani converters to a dedicated module, which is a good choice! This lightens the code of the Core.

But I'm not 100% satisfied with the name json_to_test.py. test makes me think about a unit test, here it is a test case.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right, this was confusing.

I indeed chose to separate some logic specific to turning a JSON object into a test case in a separated flag to clear the scenario code.

This is renamed, tell me if it's ok for you.

@cbenz cbenz removed their assignment Feb 6, 2017
@fpagnoux fpagnoux requested a review from cbenz February 16, 2017 17:57
@cbenz cbenz force-pushed the internationalize-scenarios branch 2 times, most recently from 012cf3f to b84ef8a Compare February 16, 2017 18:40
Copy link
Member

@cbenz cbenz left a comment

Choose a reason for hiding this comment

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

I tested this PR on my machine and it's OK.

There is still some dept as @fpagnoux said, but it's way better!

@benjello
Copy link
Member

@cbenz : did you try it on openfisca-senegal ? openfisca-tunisia ?

@fpagnoux fpagnoux force-pushed the internationalize-scenarios branch from b84ef8a to e28e59e Compare February 16, 2017 19:31
@fpagnoux
Copy link
Member Author

I tried in with openfisca-senegal. Not with tunisia.

@fpagnoux
Copy link
Member Author

fpagnoux commented Feb 16, 2017

It works with senegal.
I can help you migrate tunisia: the main task is to distinguish in scenarios which part of the JSON processing are specific to Tunisia and which part are commons.

@fpagnoux fpagnoux merged commit e50ffc3 into master Feb 16, 2017
@fpagnoux fpagnoux deleted the internationalize-scenarios branch February 16, 2017 19:37
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