Add editorial content on parameter and variable #717

sandcha · 2018-08-27T21:14:34Z

Connected to openfisca/legislation-explorer#124

New features

Allow for multiline editorial content on parameters and variables:

Introduce ParameterNode.documentation, Parameter.documentation and Variable.documentation attributes
User can access a documentation attribute through Web API
- on /parameter nodes as /parameter/benefits
  
  = python ParameterNode.documentation
  = documentation string attribute of index.yaml
- on /parameter leafs as /parameter/benefits/housing_allowance
  
  = python Parameter.documentation
  = yaml documentation string attribute
- on /variable leafs as /variable/housing_allowance
  
  = python Variable.documentation
- on every /variable leaf formula
  
  = python Variable formula docstring

Here is web API /variable/housing_allowance example:

  {
    "description": "Housing allowance",
    "documentation": "This allowance was introduced on the 1st of Jan 1980.\nIt needs the   'rent' value (same month) but doesn't care about the 'housing_occupancy_status'.",
    "entity": "household",
    "formulas": {
      "1980-01-01": {
        "content": "def formula_1980(household, period, parameters):\n    '''This is my specific life statement.'''\n    return     household('rent', period) * parameters(period).benefits.housing_allowance\n",
        "documentation": "This is my specific life statement.",
        "source": "https://github.com/openfisca/country-template/blob/3.3.0/openfisca_country_template/variables/benefits.py#L47-L49"
      },
      "2016-12-01": null
    },
    "id": "housing_allowance",
    (...)
  }

The formula doctoring is in the formula content and documentation attributes.

benjello · 2018-08-28T08:23:18Z

@sandcha : I am ok with that. My only concern is performance. Does the inclusion of an new attribute for all the variables may alter performance when instantiating a tax benefit system ?

fpagnoux · 2018-09-03T02:41:43Z

tests/web_api/test_variables.py

+    response = subject.get('/variable/housing_allowance')
+    variable = json.loads(response.data.decode('utf-8'))
+    assert_equal(variable['documentation'],
+        "\nThis allowance was introduced on the 1st of Jan 1980.\nIt needs the 'rent' value "


The first line break is not relevant, and should be removed somehow. Same for the last one.

See #714 (comment) for an easy implem

fpagnoux · 2018-09-03T02:41:46Z

openfisca_web_api/loader/variables.py

@@ -78,6 +78,9 @@ def build_variable(variable, country_package_metadata, tax_benefit_system):
            ),
        }

+    if variable.documentation:


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 :)

fpagnoux · 2018-09-03T02:42:39Z

openfisca_core/parameters.py

@@ -120,19 +120,23 @@ def __init__(self, name, data, file_path = None):
        _validate_parameter(self, data, data_type = dict)
        self.description = None
        self.metadata = {}
+        self.documentation = None


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.

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.

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.

fpagnoux · 2018-09-03T02:47:19Z

tests/core/test_parameters.py

+        'A fraction of the rent. \nFrom the 1st of Dec 2016, the housing allowance no longer exists.\n')
+
+
+@raises(AttributeError)


Even if we chose not to give parameter nodes a documentation, I'm not sure about the relevance of such a test.

When reading this test, it feels like we are testing that Python sends an AttributeError when calling an undefined attribute, which doesn't say much about OpenFisca itself

Removed

It was there to define the expected behaviour before coding but is irrelevant now.

fpagnoux · 2018-09-24T09:34:24Z

openfisca_core/parameters.py

+        :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: Some documentation describing parameter usage and context.


Some documentation -> Documentation

fpagnoux · 2018-09-24T09:43:43Z

openfisca_core/parameters.py

@@ -315,6 +320,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: Some documentation describing parameter node usage and context.


Some documentation -> Documentation

fpagnoux · 2018-09-24T09:44:52Z

openfisca_core/parameters.py

                        self.description = data.get('description', None)
+                        self.documentation = data.get('documentation', None)


None is already the default 2nd argument of get, specifying is redundant.

fpagnoux · 2018-09-24T09:45:41Z

openfisca_core/parameters.py

            self.description = data.get('description', None)
+            self.documentation = data.get('documentation', None)


Unnecessary 2nd argument

fpagnoux · 2018-09-24T09:46:26Z

openfisca_core/parameters.py

@@ -378,8 +386,9 @@ 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.


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

fpagnoux · 2018-09-24T09:50:36Z

openfisca_web_api/loader/variables.py

@@ -78,6 +82,9 @@ def build_variable(variable, country_package_metadata, tax_benefit_system):
            ),
        }

+    if variable.documentation:


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.

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__.

fpagnoux · 2018-09-24T09:51:59Z

tests/core/test_parameters.py

+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')


This last line break is not really relevant. Cleaning the trailing line breaks with a strip in parameters parsing would be nice.

Done for web API (not for python API).

fpagnoux · 2018-09-24T09:55:12Z

tests/web_api/test_variables.py

+    variable = json.loads(response.data.decode('utf-8'))
+    assert_equal(variable['documentation'],
+        "This allowance was introduced on the 1st of Jan 1980.\nIt needs the 'rent' value "
+        + "(same month) but doesn't care about the 'housing_occupancy_status'.")


The + is not necessary, Python automatically concatenates strings.

fpagnoux · 2018-09-30T21:10:32Z

tests/web_api/test_parameters.py

@@ -68,7 +68,11 @@ def test_parameter_node():
    assert_equal(response.status_code, OK)
    parameter = json.loads(response.data)
    assert_equal(sorted(list(parameter.keys())), ['description', 'documentation', 'id', 'metadata', 'source', 'subparams'])
-    assert_equal(parameter['documentation'], 'This (optional) file defines metadata for the node parameters.benefits in the parameter tree.')
+    assert_equal(parameter['documentation'],


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

sandcha · 2018-10-01T10:35:21Z

@benjello Including one attribute of primitive type for all the variables won't have significant effect on tax benefit system instantiation. For now, documentation attributes will mainly gather existing comments and have very short contents.

…I and strip its value

sandcha requested review from fpagnoux, Anna-Livia and benjello August 27, 2018 21:16

fpagnoux requested changes Sep 3, 2018

View reviewed changes

sandcha requested a review from fpagnoux September 13, 2018 14:12

sandcha mentioned this pull request Sep 14, 2018

Add editorial content on parameter and variable openfisca/country-template#53

Merged

bonjourmauko added flow:team:doing kind:feat A feature request, a feature deprecation labels Sep 19, 2018

fpagnoux requested changes Sep 24, 2018

View reviewed changes

sandcha requested a review from fpagnoux September 27, 2018 16:54

fpagnoux approved these changes Sep 30, 2018

View reviewed changes

sandcha added 3 commits October 1, 2018 12:40

Add doc attribute to Variable

1171b46

Add Variable doc attribute to web api

c934de6

Remove Variable doc indentation

f356ec3

benjello approved these changes Oct 1, 2018

View reviewed changes

sandcha force-pushed the editorialized-model-le124 branch from 8fcc772 to c638fcd Compare October 1, 2018 16:32

sandcha added 12 commits October 1, 2018 18:43

Rename Variable doc attribute to documentation

7ec3f0a

Add documentation attribute to Parameter and web api

b56d700

Test Variable and Parameter documentation attribute

731e8c3

Expose variable formulas docstrings in API documentation attribute

3a5d2bb

Add documentation attribute to ParameterNode

b24eb76

Make flake8 happy

a4f0769

Add documentation attribute on ParameterNode in Web API

fe70fb9

Clean up unnecessary code and obsolete comments

5a8748b

Update web API test on variable after country template evolution

722f702

Update web API test on parameters after country template evolution

7bd9937

Expose only existing parameter and parameter node documentation in AP…

d3f5fc0

…I and strip its value

Expose only existing variable formula documentation and strip its value

6e3bfbd

sandcha added 2 commits October 1, 2018 18:48

Bump version to 24.4.0

ebe5b78

Improve CHANGELOG and update it for ParameterNode documentation

383be19

sandcha force-pushed the editorialized-model-le124 branch from 89a4bb8 to 691fbde Compare October 1, 2018 16:48

sandcha and others added 2 commits October 2, 2018 10:44

Update country template dependency to release candidate

493122e

Improve changelog

f194591

fpagnoux force-pushed the editorialized-model-le124 branch 3 times, most recently from a438a85 to f194591 Compare October 2, 2018 08:54

fpagnoux merged commit 9f23dde into master Oct 2, 2018

fpagnoux deleted the editorialized-model-le124 branch October 2, 2018 08:56

fpagnoux removed the flow:team:doing label Oct 2, 2018

sandcha mentioned this pull request Oct 4, 2018

Transformer en documentation les commentaires python openfisca/openfisca-france#1141

Closed

fpagnoux mentioned this pull request Oct 30, 2018

Always allow documentation property for parameter nodes #753

Merged

Morendil mentioned this pull request Nov 22, 2018

Améliore revenus fonciers openfisca/openfisca-france#1216

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add editorial content on parameter and variable #717

Add editorial content on parameter and variable #717

sandcha commented Aug 27, 2018 •

edited

Loading

benjello commented Aug 28, 2018 •

edited

Loading

fpagnoux Sep 3, 2018

fpagnoux Sep 4, 2018

fpagnoux Sep 3, 2018

fpagnoux Sep 3, 2018

sandcha Sep 4, 2018

fpagnoux Sep 4, 2018 •

edited

Loading

fpagnoux Sep 3, 2018

sandcha Sep 13, 2018

fpagnoux Sep 24, 2018

fpagnoux Sep 24, 2018

fpagnoux Sep 24, 2018

fpagnoux Sep 24, 2018

fpagnoux Sep 24, 2018

fpagnoux Sep 24, 2018

sandcha Sep 27, 2018

fpagnoux Sep 24, 2018

sandcha Sep 27, 2018

fpagnoux Sep 24, 2018

fpagnoux Sep 30, 2018

sandcha commented Oct 1, 2018

		'A fraction of the rent. \nFrom the 1st of Dec 2016, the housing allowance no longer exists.\n')


		@raises(AttributeError)

		self.description = data.get('description', None)
		self.documentation = data.get('documentation', None)

Add editorial content on parameter and variable #717

Add editorial content on parameter and variable #717

Conversation

sandcha commented Aug 27, 2018 • edited Loading

New features

benjello commented Aug 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fpagnoux Sep 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandcha commented Oct 1, 2018

sandcha commented Aug 27, 2018 •

edited

Loading

benjello commented Aug 28, 2018 •

edited

Loading

fpagnoux Sep 4, 2018 •

edited

Loading