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 editorial content on parameter and variable #717

Merged
merged 19 commits into from
Oct 2, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,4 @@ dist/
.tags*
.noseids
.pytest_cache

20 changes: 19 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,22 @@
# Changelog

## 24.4.0 [#717](https://github.com/openfisca/openfisca-core/pull/717)

- In Python, allow multiline documentation on parameters and variables
- Introduce `documentation` attribute on `ParameterNode`, `Parameter` and `Variable` classes

- In the Web API, expose this documentation as a `documentation` property for parameters, variables and variables' formulas
- on `/parameter` nodes as `/parameter/benefits`
> = python `ParameterNode.documentation`
> = YAML parameter node (`index.yaml`) `documentation` string attribute
- on `/parameter` leafs as `/parameter/benefits/housing_allowance`
> = python `Parameter.documentation`
> = YAML parameter `documentation` string attribute
- on `/variable` as `/variable/housing_allowance`
> = python `Variable.documentation`
- on every `/variable` leaf formula
> = python `Variable` formula **docstring**

### 24.3.2 [#727](https://github.com/openfisca/openfisca-core/pull/727)

- Add a style formatter that follows community code conventions
Expand Down Expand Up @@ -52,7 +69,7 @@ from openfisca_core.tools.simulation_dumper import restore_simulation
simulation = restore_simulation('/path/to/directory', tax_benefit_system)
```

## 24.1.0 [#713](https://github.com/openfisca/openfisca-core/pull/713)
### 24.1.0 [#713](https://github.com/openfisca/openfisca-core/pull/713)

- Enhance navigation within the Openfisca Web API.
- Provides a direct link to individual parameters and variables from the `/parameters` and `/variables` routes.
Expand Down Expand Up @@ -82,6 +99,7 @@ becomes:
},
...
```

### 24.0.1 [#711](https://github.com/openfisca/openfisca-core/pull/711)

- Fix spelling in warning about libyaml
Expand Down
30 changes: 19 additions & 11 deletions openfisca_core/parameters.py
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,10 @@ class Parameter(object):
"""
A parameter of the legislation. Parameters can change over time.

:param name: name of the parameter, e.g. "taxes.some_tax.some_param"
:param data: Data loaded from a YAML file.
:param file_path: File the parameter was loaded from.
:param string name: Name of the parameter, e.g. "taxes.some_tax.some_param"
:param dict data: Data loaded from a YAML file.
:param string file_path: File the parameter was loaded from.
:param string documentation: Documentation describing parameter usage and context.


Instantiate a parameter without metadata:
Expand Down Expand Up @@ -121,19 +122,23 @@ def __init__(self, name, data, file_path = None):
_validate_parameter(self, data, data_type = dict)
self.description = None
self.metadata = {}
self.documentation = None
Copy link
Member

Choose a reason for hiding this comment

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

Should we also add this new attributes to parameter nodes? I'm not sure there is a good reason to do it only for leads.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you see some use case for documentation on ParameterNodeobjects?
As I see it, for now, we wouldn't use it so it would be over-engineering it.

Copy link
Member

@fpagnoux fpagnoux Sep 4, 2018

Choose a reason for hiding this comment

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

Yes, imagine something like: rsa.plafonds.1_enfant, rsa.plafonds.2_enfants, rsa.plafonds.par_enfant_supp.

In that case, it's probably more interesting to document the rsa.plafonds rather than the specific values.

In many case, parameters work by group and are not semantically fully independent. In that case, the nodes may be the thing we want to document.

self.values_history = self # Only for backward compatibility

# Normal parameter declaration: the values are declared under the 'values' key: parse the description and metadata.
if data.get('values'):
# 'unit' and 'reference' are only listed here for backward compatibility
_validate_parameter(self, data, allowed_keys = set(['values', 'description', 'metadata', 'unit', 'reference']))
_validate_parameter(self, data, allowed_keys = set(['values', 'description', 'metadata', 'unit', 'reference', 'documentation']))
self.description = data.get('description')

_set_backward_compatibility_metadata(self, data)
self.metadata.update(data.get('metadata', {}))

_validate_parameter(self, data['values'], data_type = dict)
values = data['values']

self.documentation = data.get('documentation')

else: # Simplified parameter declaration: only values are provided
values = data

Expand Down Expand Up @@ -254,9 +259,9 @@ class ParameterAtInstant(object):

def __init__(self, name, instant_str, data = None, file_path = None, metadata = None):
"""
:param name: name of the parameter, e.g. "taxes.some_tax.some_param"
:param instant_str: Date of the value in the format `YYYY-MM-DD`.
:param data: Data, usually loaded from a YAML file.
:param string name: name of the parameter, e.g. "taxes.some_tax.some_param"
:param string instant_str: Date of the value in the format `YYYY-MM-DD`.
:param dict data: Data, usually loaded from a YAML file.
"""
self.name = name
self.instant_str = instant_str
Expand Down Expand Up @@ -316,6 +321,7 @@ def __init__(self, name = "", directory_path = None, data = None, file_path = No
:param string name: Name of the node, eg "taxes.some_tax".
:param string directory_path: Directory containing YAML files describing the node.
:param dict data: Object representing the parameter node. It usually has been extracted from a YAML file.
:param string documentation: Documentation describing parameter node usage and context.
:param string file_path: YAML file from which the `data` has been extracted from.


Expand Down Expand Up @@ -343,6 +349,7 @@ def __init__(self, name = "", directory_path = None, data = None, file_path = No
self.name = name
self.children = {}
self.description = None
self.documentation = None
self.file_path = None
self.metadata = {}

Expand All @@ -359,8 +366,9 @@ def __init__(self, name = "", directory_path = None, data = None, file_path = No

if child_name == 'index':
data = _load_yaml_file(child_path)
_validate_parameter(self, data, allowed_keys = ['metadata', 'description', 'reference'])
self.description = data.get('description', None)
_validate_parameter(self, data, allowed_keys = ['metadata', 'description', 'documentation', 'reference'])
self.description = data.get('description')
self.documentation = data.get('documentation')
_set_backward_compatibility_metadata(self, data)
self.metadata.update(data.get('metadata', {}))
else:
Expand All @@ -379,8 +387,8 @@ def __init__(self, name = "", directory_path = None, data = None, file_path = No
else:
self.file_path = file_path
_validate_parameter(self, data, data_type = dict, allowed_keys = self._allowed_keys)
# We allow to set a reference and a description for a node.
Copy link
Member

Choose a reason for hiding this comment

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

This comment seems outdated (we don't set a reference around it). I'd just remove it 🙂

self.description = data.get('description', None)
self.description = data.get('description')
self.documentation = data.get('documentation')
_set_backward_compatibility_metadata(self, data)
self.metadata.update(data.get('metadata', {}))
for child_name, child in data.items():
Expand Down
9 changes: 9 additions & 0 deletions openfisca_core/variables.py
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,10 @@ class Variable(object):
.. py:attribute:: unit

Free text field describing the unit of the variable. Only used as metadata.

.. py:attribute:: documentation

Free multilines text field describing the variable context and usage.
"""

def __init__(self, baseline_variable = None):
Expand Down Expand Up @@ -175,6 +179,7 @@ def __init__(self, baseline_variable = None):
self.reference = self.set(attr, 'reference', setter = self.set_reference)
self.cerfa_field = self.set(attr, 'cerfa_field', allowed_type = (basestring_type, dict))
self.unit = self.set(attr, 'unit', allowed_type = basestring_type)
self.documentation = self.set(attr, 'documentation', allowed_type = basestring_type, setter = self.set_documentation)
self.set_input = self.set_set_input(attr.pop('set_input', None))
self.calculate_output = self.set_calculate_output(attr.pop('calculate_output', None))
self.is_period_size_independent = self.set(attr, 'is_period_size_independent', allowed_type = bool, default = VALUE_TYPES[self.value_type]['is_period_size_independent'])
Expand Down Expand Up @@ -255,6 +260,10 @@ def set_reference(self, reference):

return reference

def set_documentation(self, documentation):
if documentation:
return textwrap.dedent(documentation)

def set_base_function(self, base_function):
if not base_function and self.baseline_variable:
return self.baseline_variable.base_function
Expand Down
4 changes: 4 additions & 0 deletions openfisca_web_api/loader/parameters.py
Original file line number Diff line number Diff line change
Expand Up @@ -76,10 +76,14 @@ def walk_node(node, parameters, path_fragments, country_package_metadata):
if child.file_path:
api_parameter['source'] = build_source_url(child.file_path, country_package_metadata)
if isinstance(child, Parameter):
if child.documentation:
api_parameter['documentation'] = child.documentation.strip()
api_parameter['values'] = build_api_values_history(child)
elif isinstance(child, Scale):
api_parameter['brackets'] = build_api_scale(child)
elif isinstance(child, ParameterNode):
if child.documentation:
api_parameter['documentation'] = child.documentation.strip()
api_parameter['subparams'] = {
grandchild_name: {
'description': grandchild.description,
Expand Down
12 changes: 11 additions & 1 deletion openfisca_web_api/loader/variables.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,10 @@ def build_formula(formula, country_package_metadata, source_file_path, tax_benef
# Python 2 backward compatibility
if isinstance(source_code[0], bytes):
source_code = [source_line.decode('utf-8') for source_line in source_code]

source_code = textwrap.dedent(''.join(source_code))
return {

api_formula = {
'source': build_source_url(
country_package_metadata,
source_file_path,
Expand All @@ -54,6 +56,11 @@ def build_formula(formula, country_package_metadata, source_file_path, tax_benef
'content': to_unicode(source_code),
}

if formula.__doc__:
api_formula['documentation'] = to_unicode(textwrap.dedent(formula.__doc__))

return api_formula


def build_formulas(formulas, country_package_metadata, source_file_path, tax_benefit_system):
return {
Expand All @@ -79,6 +86,9 @@ def build_variable(variable, country_package_metadata, tax_benefit_system):
),
}

if variable.documentation:
Copy link
Member

Choose a reason for hiding this comment

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

Looking at OpenFisca France, a lot of doc string is added at the formula level. This PR is not exposing this doc though the API. We should probably do that too :)

Copy link
Member

Choose a reason for hiding this comment

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

The way we handle unspecified documentation in the Web API is currently inconsistent:

  • documentation: null for parameters
  • documentation: "" for formulas
  • no documentation key for variables

I'd be in favor of generalizing the 3rd option, as it makes the JSON more readable, lighter, and doesn't change anything for JS clients.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed for Web API : a documentation attribute is exposed only when it exists and its value is cleaned from start and end spaces.

On Python API : the documentation attribute value is not modified at all.
For variable formulas, there is no specific documentation attribute as the user can already get formula docstring with formula.__doc__.

result['documentation'] = variable.documentation.strip()

if variable.reference:
result['references'] = variable.reference

Expand Down
4 changes: 2 additions & 2 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,13 @@
'flake8 >= 3.5.0, < 3.6.0',
'autopep8 >= 1.4.0, < 1.5.0',
'pycodestyle < 2.4.0',
'openfisca-country-template >= 3.2.3, < 4.0.0',
'openfisca-country-template >= 3.3.1rc1, < 4.0.0',
'openfisca-extension-template >= 1.1.3, < 2.0.0',
] + api_requirements

setup(
name = 'OpenFisca-Core',
version = '24.3.2',
version = '24.4.0',
author = 'OpenFisca Team',
author_email = 'contact@openfisca.org',
classifiers = [
Expand Down
6 changes: 6 additions & 0 deletions tests/core/test_parameters.py
Original file line number Diff line number Diff line change
Expand Up @@ -94,3 +94,9 @@ def test_parameter_node_metadata():

parameter_2 = tax_benefit_system.parameters.taxes.housing_tax
assert_equal(parameter_2.description, 'Housing tax')


def test_parameter_documentation():
parameter = tax_benefit_system.parameters.benefits.housing_allowance
assert_equal(parameter.documentation,
'A fraction of the rent.\nFrom the 1st of Dec 2016, the housing allowance no longer exists.\n')
16 changes: 14 additions & 2 deletions tests/web_api/test_parameters.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,16 +61,28 @@ def test_parameter_values():
assert_regexp_matches(parameter['source'], GITHUB_URL_REGEX)
assert_in('taxes/income_tax_rate.yaml', parameter['source'])

# 'documentation' attribute exists only when a value is defined
response = subject.get('/parameter/benefits/housing_allowance')
parameter = json.loads(response.data)
assert_equal(sorted(list(parameter.keys())), ['description', 'documentation', 'id', 'metadata', 'source', 'values'])
assert_equal(parameter['documentation'],
'A fraction of the rent.\nFrom the 1st of Dec 2016, the housing allowance no longer exists.')


def test_parameter_node():
response = subject.get('/parameter/benefits')
assert_equal(response.status_code, OK)
parameter = json.loads(response.data)
assert_equal(sorted(list(parameter.keys())), ['description', 'id', 'metadata', 'source', 'subparams'])
assert_equal(sorted(list(parameter.keys())), ['description', 'documentation', 'id', 'metadata', 'source', 'subparams'])
assert_equal(parameter['documentation'],
Copy link
Member

Choose a reason for hiding this comment

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

For test robustness sake, I'd just check that parameter['documentation'] contains a key expression, like "Government support for the citizens and residents".

"Government support for the citizens and residents of society. "
"\nThey may be provided to people of any income level, as with social security, "
"\nbut usually it is intended to ensure that everyone can meet their basic human needs "
"\nsuch as food and shelter.\n(See https://en.wikipedia.org/wiki/Welfare)")
assert_equal(parameter['subparams'], {
'housing_allowance': {'description': 'Housing allowance amount (as a fraction of the rent)'},
'basic_income': {'description': 'Amount of the basic income'}
})
}, parameter['subparams'])


def test_stopped_parameter_values():
Expand Down
10 changes: 10 additions & 0 deletions tests/web_api/test_variables.py
Original file line number Diff line number Diff line change
Expand Up @@ -154,3 +154,13 @@ def test_dated_variable_formulas_content():
def test_variable_encoding():
variable_response = subject.get('/variable/pension')
assert_equal(variable_response.status_code, OK)


def test_variable_documentation():
response = subject.get('/variable/housing_allowance')
variable = json.loads(response.data.decode('utf-8'))
assert_equal(variable['documentation'],
"This allowance was introduced on the 1st of Jan 1980.\nIt disappeared in Dec 2016.")

assert_equal(variable['formulas']['1980-01-01']['documentation'],
"\nTo compute this allowance, the 'rent' value must be provided for the same month, but 'housing_occupancy_status' is not necessary.\n")