[3/17] Add tests to entities #1192
Conversation
bonjourmauko
added
meta:quality-assurance
level:starter
| plural: str, | ||
| label: str, | ||
| doc: str, | ||
| roles: Sequence[HasKey], |
There was a problem hiding this comment.
Is there any reason why the roles should be ordered? I don't see one and would recommend to use Set or Iterable instead.
There was a problem hiding this comment.
I would not change that ionce it might be used by package dependent on core (and even core) when filling population if the roles are indexed by an integer.
There was a problem hiding this comment.
After spending a couple of hours on this... good part of the code depends on roles and sub-roles being ordered.
There was a problem hiding this comment.
I prefer not to touch that for now, not before the aforesaid parts of the code are properly tested and documented.
There was a problem hiding this comment.
BTW @MattiSG at least in Python, iterable is a generic protocol including list (ordered, mutable), tuple (ordered, immutable), dict (not-ordered, mutable), and set (not-ordered, sort of mutable).
@benjello Why? You mean, for example, I have a GroupEntity of 4 people. You'll use role/subrole's index to determine each person's order within the group? (I actually had strange results when I tried to remove indexation). A lot to do in OpenFisca yet! 😄
|
@benjello This PR opened a couple of questions that I pose here for documentation purposes:
These are all breaking-changes, but refactoring each could greatly facilitate maintenance and new features. |
openfisca_core/entities/entity.py
Outdated
| @@ -46,10 +46,10 @@ def check_variable_defined_for_entity(self, variable_name: str) -> None: | |||
| if entity.key != self.key: | |||
| message = os.linesep.join( | |||
| [ | |||
| "You tried to compute the variable '{0}' for the entity '{1}';".format( | |||
| "You tried to compute the variable '{}' for the entity '{}';".format( | |||
There was a problem hiding this comment.
What about using f-stings here too ?
openfisca_core/entities/entity.py
Outdated
| variable_name, self.plural | ||
| ), | ||
| "however the variable '{0}' is defined for '{1}'.".format( | ||
| "however the variable '{}' is defined for '{}'.".format( |
There was a problem hiding this comment.
What about using f-stings here too ?
bonjourmauko
force-pushed
the
compose-entity
branch
from
ed0fca8
to
696e00f
Compare
Technical changes
entities.