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

Add situation example #38

Merged
merged 14 commits into from
May 28, 2018
Merged

Add situation example #38

merged 14 commits into from
May 28, 2018

Conversation

jvalduvieco
Copy link
Contributor

  • Documentation improvement
  • Details:
    • Add a situation test example as in openfisca-barcelona is the main testing process and there is no example in the country template.
  • Found a missing parameter in pension.

@fpagnoux
Copy link
Member

Arg, didn't see this PR, sorry @jvalduvieco, and thanks @MattiSG for asking the review.

Will have a look at it tomorrow :).

@fpagnoux
Copy link
Member

Sorry, I'm running out of time at the moment, but I swear I haven't forgotten about this PR :)

Copy link
Member

@MattiSG MattiSG left a comment

Choose a reason for hiding this comment

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

Thank you so much @jvalduvieco! I agree that not having an example of situation with multiple Persons has been confusing for newcomers. It feels like it's not possible to do such tests!

The feedback I have given is very extreme, as the country template has to be a “best practice” showcase, so we have to make sure it shows the very best recommendations.

I would totally understand if you did not have the time to take these change suggestions into account. If you don't feel like it, I can definitely take care of these small syntax changes for you as additional commits.
Let me know what's the best for you, and thanks again for your contribution!! 😃

salary:
2017-01: 2400
- id: "child1"
birth: '2002-01-15'
Copy link
Member

Choose a reason for hiding this comment

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

As we want to give best practice examples, it seems important to me that we use a coherent style of quotes. If possible, no quotes at all would be the best if YAML does support it 🙂

birth: '1961-01-15'
salary:
2017-01: 2400
- id: "child1"
Copy link
Member

Choose a reason for hiding this comment

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

Best practice: give real names in examples.

@@ -0,0 +1,21 @@
- name: "A single parent with a child situation"
description: A simple situation just to test
Copy link
Member

Choose a reason for hiding this comment

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

As a best practice example, we should be as clear and literate as possible (or not add a description at all). Maybe this test is about “Income tax should get properly computed across all members of a household”? 😃

absolute_error_margin: 0
households:
- parents: ["parent1"]
children: ["child1"]
Copy link
Member

@MattiSG MattiSG May 19, 2018

Choose a reason for hiding this comment

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

I believe best readability practice includes internal spaces: [ parent1 ] is more readable than ["parent1"] 🙂

@jvalduvieco
Copy link
Contributor Author

Comments fixed. :)

@MattiSG
Copy link
Member

MattiSG commented May 28, 2018

Brilliant, thanks a lot @jvalduvieco!

@MattiSG MattiSG merged commit d389c91 into openfisca:master May 28, 2018
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.

4 participants