Add a `/entities` endpoint to the API #714

Anna-Livia · 2018-08-23T14:16:08Z

Fixes #561

New features

Get information on a country package's entities, and their roles
Introduce the /entities endpoint for the Web API

{
            "description": "[...]",
            "plural": "households",
            "roles": {
                "parent": {
                    "description": "The one or two adults in charge of the household.",
                    "mandatory": "true",
                    "max": 2,
                    "plural": "parents"
                    },
                "child": {
                    "description": "Other individuals living in the household.",
                    "mandatory": "false",
                    "plural": "children"
                    }
                }
            }

fpagnoux

Nice 👍 !

We're finally one step closer from documenting the entities in the legislation explorer 🎉

fpagnoux · 2018-08-27T19:12:22Z

CHANGELOG.md

+- Introduce the `/entities` endpoint for the Web API
+```json
+{
+            "description": "[...]",


Could we make up a simple description? Examples will real values are always easier to grasp 🙂

Ok, in my previous comment I thought description was the label, now I understand better why you didn't want to write the full Household doc.
But I still think a one or two-sentence max doc would hep :)

fpagnoux · 2018-08-27T19:15:49Z

openfisca_web_api/loader/entities.py

+
+    entity_formated = {
+        'plural': entity.plural,
+        'description': to_unicode(entity.doc)


#561 mentioned a documentation key containing the docstring.
If we are to have a description key, it should probably contain entity.label

fpagnoux · 2018-08-27T19:17:08Z

openfisca_web_api/loader/entities.py

+        'description': to_unicode(entity.doc)
+        }
+    if hasattr(entity, 'roles'):
+        entity_formated['roles'] = \


Avoid \ for line continuation: you can rely on implicit continuation, as you've done everywhere else 🙂

I think, pycharm outsmarted me. ;)

fpagnoux · 2018-08-27T19:17:56Z

openfisca_web_api/loader/entities.py

+        'plural': entity.plural,
+        'description': to_unicode(entity.doc)
+        }
+    if hasattr(entity, 'roles'):


We usually use:

if entity.is_person:

Can't a group entity not have roles ?
it seems to me that if entity.is_person: would give me all group entities whether they have rolesor not 🤔

Fair point.

We have never used a group entity without roles, and right now it would trigger an ugly error, but you're right, there could be use cases.

However, even if we had a group entity without roles, it should still have an attribute roles that would be [], so the code further down would still work. In general, using hasattr is pretty rare unless you are doing relatively tricky stuff.

fpagnoux · 2018-08-27T19:19:33Z

openfisca_web_api/loader/entities.py

+    if role.max:
+        role_formated['max'] = role.max
+    if role.subroles:
+        role_formated['max'] = len(role.subroles)


fpagnoux · 2018-08-27T19:24:27Z

openfisca_web_api/loader/entities.py

+    if role.subroles:
+        role_formated['max'] = len(role.subroles)
+
+    role_formated['mandatory'] = True if role_formated.get('max') else False


No, there is no good reason to assume than a role with a max is a mandatory role.

Right now, we don't handle mandatory role in OpenFisca Core. I see two solutions to deal with that:

Always set role_formated['mandatory'] to False (the Web API would be "anticipating" the feature)

Open another PR in Core introducing this mandatory attribute in Python

1 allows for a quick delivery, but leads to a relatively inconsistent state of the codebase.

I implemented 1 and opened #721 to follow 2

fpagnoux · 2018-08-27T19:25:37Z

openfisca_web_api/openAPI.yml

@@ -16,13 +16,18 @@ tags:
  - name: "Parameters"
    description: "A parameter is a numeric property of the legislation that can evolve over time."
    externalDocs:
-      description: "Parameter official documentation"
+      description: "Official Parameter documentation"


Not we sure what we mean by "official". Never seen an fan-based documentation so far, nor an off-brand one ;)

fpagnoux · 2018-08-27T19:28:17Z

openfisca_web_api/loader/entities.py

+
+def build_entity(entity):
+
+    entity_formated = {


formatted_entity ?

fpagnoux · 2018-08-27T19:31:25Z

tests/web_api/test_entities.py

+
+def test_response_data():
+    entities = json.loads(entities_response.data.decode('utf-8'))
+    test_description = '''


Copy-pasting the description makes the test really fragile. A minor edition of the doc would break it.
I'd recommend importing Household from the country template and getting the doc from Household.doc

fpagnoux · 2018-08-27T19:32:00Z

tests/web_api/test_entities.py

+            'roles': {
+                'parent': {
+                    'description': 'The one or two adults in charge of the household.',
+                    'mandatory': True,


Should probably stick to False unless we actually handle mandatory roles.

fpagnoux

Sorry, there is a couple of things I missed during my first review, but we are almost there :) !

fpagnoux · 2018-09-03T01:51:34Z

openfisca_web_api/openAPI.yml

+    additionalProperties:
+      type: "object"
+      properties:
+        definition:


I actually don't think there is a definition property for our entities object. We should also document the following properties:

description

mandatory

max

plural

documentation

and roles ?

Yep, forgot that one 😓

fpagnoux · 2018-09-03T01:57:56Z

openfisca_web_api/loader/entities.py

+    if role.subroles:
+        formatted_role['max'] = len(role.subroles)
+
+    formatted_role['mandatory'] = False


The more I think about it, the least confortable I feel about this mandatory property.

For instance, are we sure the following

max: 2 mandatory: true

is clearer than

max:2 min:1

?

I'd be in favor of removing that property and postponing this discussion to #721

I totally agree

fpagnoux · 2018-09-03T02:50:09Z

openfisca_web_api/loader/entities.py

+    formatted_entity = {
+        'plural': entity.plural,
+        'description': to_unicode(entity.label),
+        'documentation': to_unicode(entity.doc)


documentation seems to often start and end with \n, which is not really relevant. We should probably clean these :)

I found a way but it is not very elegant.... I'm looking forward to your feedback on it :)

fpagnoux · 2018-09-04T11:30:00Z

openfisca_web_api/loader/entities.py

+    split_doc = to_unicode(entity.doc).split("\n")
+    formatted_start_doc = split_doc[1:] if split_doc[0] == "" else split_doc
+    formatted_end_doc = formatted_start_doc[:-1] if formatted_start_doc[-1] == "" else formatted_start_doc
+    formatted_doc = "\n".join(formatted_end_doc)


I think you can just use formatted_doc = to_unicode(entity.doc.strip())

strip removes all trailing white spaces, and \n is considered as a particular white space 🎉

fpagnoux

build_entity should be simplified, but GTM otherwise 🙂

Anna-Livia requested review from fpagnoux and sandcha August 23, 2018 16:28

fpagnoux requested changes Aug 27, 2018

View reviewed changes

Anna-Livia mentioned this pull request Aug 31, 2018

Set max and min amount of person with a given role in entities specification #721

Closed

Anna-Livia requested a review from fpagnoux August 31, 2018 09:05

fpagnoux requested changes Sep 3, 2018

View reviewed changes

fpagnoux reviewed Sep 3, 2018

View reviewed changes

Anna-Livia requested a review from fpagnoux September 4, 2018 08:34

fpagnoux reviewed Sep 4, 2018

View reviewed changes

fpagnoux approved these changes Sep 4, 2018

View reviewed changes

Create route

cd4716f

Anna-Livia force-pushed the entities branch from 03e66a4 to a794fc4 Compare September 4, 2018 13:45

Anna-Livia added 11 commits September 4, 2018 16:38

Display key and plurals from entities

9c5ab0c

Add support for description

6dd3609

Display sub-roles

4b30f19

Ensure P2/P3 compatibility

1dd5294

Make variable names more explicit

c18bcd5

Make tests more robust

97aea2e

Format documentation string

affce9a

Take out mandatory attribute

7e408ef

Update openAPI specs

4b0d782

Update .gitignore for pytest

a95e11b

Version bump

082625d

Anna-Livia force-pushed the entities branch from a794fc4 to 082625d Compare September 4, 2018 14:39

Anna-Livia merged commit 7aeadae into master Sep 4, 2018

fpagnoux mentioned this pull request Sep 4, 2018

Add editorial content on parameter and variable #717

Merged

bonjourmauko added the kind:feat A feature request, a feature deprecation label Sep 20, 2018

guillett deleted the entities branch November 15, 2018 07:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a `/entities` endpoint to the API #714

Add a `/entities` endpoint to the API #714

Anna-Livia commented Aug 23, 2018 •

edited

Loading

fpagnoux left a comment

fpagnoux Aug 27, 2018

fpagnoux Aug 27, 2018

fpagnoux Aug 27, 2018

fpagnoux Aug 27, 2018

Anna-Livia Aug 30, 2018

fpagnoux Aug 27, 2018

Anna-Livia Aug 30, 2018

fpagnoux Aug 30, 2018 •

edited

Loading

fpagnoux Aug 27, 2018

fpagnoux Aug 27, 2018

Anna-Livia Aug 31, 2018

fpagnoux Aug 27, 2018

fpagnoux Aug 27, 2018

fpagnoux Aug 27, 2018

fpagnoux Aug 27, 2018

fpagnoux left a comment

fpagnoux Sep 3, 2018

Anna-Livia Sep 3, 2018

fpagnoux Sep 4, 2018

fpagnoux Sep 3, 2018

Anna-Livia Sep 4, 2018

fpagnoux Sep 3, 2018

Anna-Livia Sep 4, 2018

fpagnoux Sep 4, 2018

Anna-Livia Sep 4, 2018

fpagnoux left a comment

Add a /entities endpoint to the API #714

Add a /entities endpoint to the API #714

Conversation

Anna-Livia commented Aug 23, 2018 • edited Loading

New features

fpagnoux left a 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

fpagnoux Aug 30, 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

fpagnoux left a 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

fpagnoux left a comment

Choose a reason for hiding this comment

Add a `/entities` endpoint to the API #714

Add a `/entities` endpoint to the API #714

Anna-Livia commented Aug 23, 2018 •

edited

Loading

fpagnoux Aug 30, 2018 •

edited

Loading