From 74aff09e3c961a4dddeb42fc870b257034b001fb Mon Sep 17 00:00:00 2001 From: Mauko Quiroga Date: Fri, 25 Oct 2024 06:00:58 +0200 Subject: [PATCH 1/7] BREAKING CHANGE: remove CoreEntity.check_role_validity The method in itself had no relation to `entities` (it was a static method, so no depending neither on an entity class nor an instance). It could have been kept as a `populations` helper, where it was exclusively used. Nonetheless, `check_role_validity` had the peculiar behaviour of not failing but when the value of `role` is not `None`, that granted it in my opinion the right to be removed so as to avoid confusion. --- openfisca_core/entities/__init__.py | 2 -- openfisca_core/entities/_core_entity.py | 26 ------------------- .../populations/group_population.py | 16 +++++++++--- openfisca_core/populations/population.py | 10 ++++--- openfisca_core/types.py | 1 - 5 files changed, 19 insertions(+), 36 deletions(-) diff --git a/openfisca_core/entities/__init__.py b/openfisca_core/entities/__init__.py index 1811e3fe9..fb00bc92f 100644 --- a/openfisca_core/entities/__init__.py +++ b/openfisca_core/entities/__init__.py @@ -8,7 +8,6 @@ from .role import Role SingleEntity = Entity -check_role_validity = CoreEntity.check_role_validity __all__ = [ "CoreEntity", @@ -17,7 +16,6 @@ "Role", "SingleEntity", "build_entity", - "check_role_validity", "find_role", "types", ] diff --git a/openfisca_core/entities/_core_entity.py b/openfisca_core/entities/_core_entity.py index 33002e9af..6a6789ac5 100644 --- a/openfisca_core/entities/_core_entity.py +++ b/openfisca_core/entities/_core_entity.py @@ -6,7 +6,6 @@ import os from . import types as t -from .role import Role class CoreEntity: @@ -189,30 +188,5 @@ def check_variable_defined_for_entity(self, variable_name: t.VariableName) -> No ) raise ValueError(os.linesep.join(message)) - @staticmethod - def check_role_validity(role: object) -> None: - """Check if ``role`` is an instance of ``Role``. - - Args: - role: Any object. - - Raises: - ValueError: When ``role`` is not a ``Role``. - - Examples: - >>> from openfisca_core import entities - - >>> role = entities.Role({"key": "key"}, object()) - >>> entities.check_role_validity(role) - - >>> entities.check_role_validity("hey!") - Traceback (most recent call last): - ValueError: hey! is not a valid role - - """ - if role is not None and not isinstance(role, Role): - msg = f"{role} is not a valid role" - raise ValueError(msg) - __all__ = ["CoreEntity"] diff --git a/openfisca_core/populations/group_population.py b/openfisca_core/populations/group_population.py index 120dc9c65..1f63514e0 100644 --- a/openfisca_core/populations/group_population.py +++ b/openfisca_core/populations/group_population.py @@ -107,7 +107,9 @@ def sum(self, array, role=None): >>> array([3500]) """ - self.entity.check_role_validity(role) + if role is not None and not isinstance(role, entities.Role): + msg = f"{role} is not a valid role" + raise TypeError(msg) self.members.check_array_compatible_with_entity(array) if role is not None: role_filter = self.members.has_role(role) @@ -140,7 +142,9 @@ def any(self, array, role=None): @projectors.projectable def reduce(self, array, reducer, neutral_element, role=None): self.members.check_array_compatible_with_entity(array) - self.entity.check_role_validity(role) + if role is not None and not isinstance(role, entities.Role): + msg = f"{role} is not a valid role" + raise TypeError(msg) position_in_entity = self.members_position role_filter = self.members.has_role(role) if role is not None else True filtered_array = numpy.where(role_filter, array, neutral_element) @@ -260,7 +264,9 @@ def value_from_person(self, array, role, default=0): The result is a vector which dimension is the number of entities """ - self.entity.check_role_validity(role) + if role is not None and not isinstance(role, entities.Role): + msg = f"{role} is not a valid role" + raise TypeError(msg) if role.max != 1: msg = f"You can only use value_from_person with a role that is unique in {self.key}. Role {role.key} is not unique." raise Exception( @@ -312,7 +318,9 @@ def value_from_first_person(self, array): def project(self, array, role=None): self.check_array_compatible_with_entity(array) - self.entity.check_role_validity(role) + if role is not None and not isinstance(role, entities.Role): + msg = f"{role} is not a valid role" + raise TypeError(msg) if role is None: return array[self.members_entity_id] role_condition = self.members.has_role(role) diff --git a/openfisca_core/populations/population.py b/openfisca_core/populations/population.py index 24742ab0a..97714aa16 100644 --- a/openfisca_core/populations/population.py +++ b/openfisca_core/populations/population.py @@ -2,7 +2,7 @@ import numpy -from openfisca_core import projectors +from openfisca_core import entities, projectors from . import types as t from ._core_population import CorePopulation @@ -49,7 +49,9 @@ def has_role(self, role: t.Role) -> None | t.BoolArray: if self.simulation is None: return None - self.entity.check_role_validity(role) + if role is not None and not isinstance(role, entities.Role): + msg = f"{role} is not a valid role" + raise TypeError(msg) group_population = self.simulation.get_population(role.entity.plural) @@ -68,7 +70,9 @@ def value_from_partner( role: t.Role, ) -> None | t.FloatArray: self.check_array_compatible_with_entity(array) - self.entity.check_role_validity(role) + if role is not None and not isinstance(role, entities.Role): + msg = f"{role} is not a valid role" + raise TypeError(msg) if not role.subroles or len(role.subroles) != 2: msg = "Projection to partner is only implemented for roles having exactly two subroles." diff --git a/openfisca_core/types.py b/openfisca_core/types.py index 9c8105741..fe94b88d7 100644 --- a/openfisca_core/types.py +++ b/openfisca_core/types.py @@ -74,7 +74,6 @@ class CoreEntity(Protocol): key: EntityKey plural: EntityPlural - def check_role_validity(self, role: object, /) -> None: ... def check_variable_defined_for_entity( self, variable_name: VariableName, From 8d5deb9bd0acf46feb57eb5fef25372f6cfc46d3 Mon Sep 17 00:00:00 2001 From: Mauko Quiroga Date: Fri, 25 Oct 2024 06:17:15 +0200 Subject: [PATCH 2/7] BREAKING CHANGE: remove `class_override` arg from `entities.build_entity` That argument actually did nothing, and linters have been complaining about this argument for almost 10 years now. It is only being marked as a breaking change for respect to the semantic versioning protocol. --- openfisca_core/entities/helpers.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/openfisca_core/entities/helpers.py b/openfisca_core/entities/helpers.py index 1dcdad88a..36e90292c 100644 --- a/openfisca_core/entities/helpers.py +++ b/openfisca_core/entities/helpers.py @@ -15,7 +15,6 @@ def build_entity( roles: None | Sequence[t.RoleParams] = None, is_person: bool = False, *, - class_override: object = None, containing_entities: Sequence[str] = (), ) -> t.SingleEntity | t.GroupEntity: """Build an ``Entity`` or a ``GroupEntity``. @@ -27,7 +26,6 @@ def build_entity( doc: A full description. roles: A list of roles —if it's a ``GroupEntity``. is_person: If is an individual, or not. - class_override: ? containing_entities: Keys of contained entities. Returns: From c9eb25151740a7e1088710b89fb112831b11a74d Mon Sep 17 00:00:00 2001 From: Mauko Quiroga Date: Fri, 25 Oct 2024 07:20:48 +0200 Subject: [PATCH 3/7] BREAKING CHANGE: remove `CoreEntity.set_tax_benefit_system` The feature is now provided by simple variable assignment: ```python entity.tax_benefit_system = tax_benefit_system ``` Likewise, `entity.tax_benefit_system` becomes part of the public API (which implicitly it was as the private marker was being violated). --- openfisca_core/entities/_core_entity.py | 27 ++++++++----------- .../populations/_core_population.py | 4 +-- .../taxbenefitsystems/tax_benefit_system.py | 4 +-- openfisca_core/types.py | 2 ++ .../tools/test_runner/test_yaml_runner.py | 2 +- 5 files changed, 18 insertions(+), 21 deletions(-) diff --git a/openfisca_core/entities/_core_entity.py b/openfisca_core/entities/_core_entity.py index 6a6789ac5..cb16450da 100644 --- a/openfisca_core/entities/_core_entity.py +++ b/openfisca_core/entities/_core_entity.py @@ -44,7 +44,7 @@ class CoreEntity: is_person: ClassVar[bool] #: A ``TaxBenefitSystem`` instance. - _tax_benefit_system: None | t.TaxBenefitSystem = None + tax_benefit_system: None | t.TaxBenefitSystem = None @abc.abstractmethod def __init__(self, *__args: object, **__kwargs: object) -> None: ... @@ -52,10 +52,6 @@ def __init__(self, *__args: object, **__kwargs: object) -> None: ... def __repr__(self) -> str: return f"{self.__class__.__name__}({self.key})" - def set_tax_benefit_system(self, tax_benefit_system: t.TaxBenefitSystem) -> None: - """A ``CoreEntity`` belongs to a ``TaxBenefitSystem``.""" - self._tax_benefit_system = tax_benefit_system - def get_variable( self, variable_name: t.VariableName, @@ -92,7 +88,7 @@ def get_variable( ValueError: You must set 'tax_benefit_system' before calling this... >>> tax_benefit_system = taxbenefitsystems.TaxBenefitSystem([this]) - >>> this.set_tax_benefit_system(tax_benefit_system) + >>> this.tax_benefit_system = tax_benefit_system >>> this.get_variable("tax") @@ -105,19 +101,18 @@ def get_variable( ... value_type = float ... entity = that - >>> this._tax_benefit_system.add_variable(tax) + >>> this.tax_benefit_system.add_variable(tax) >>> this.get_variable("tax") + """ - if self._tax_benefit_system is None: - msg = "You must set 'tax_benefit_system' before calling this method." - raise ValueError( - msg, - ) - return self._tax_benefit_system.get_variable(variable_name, check_existence) + if self.tax_benefit_system is None: + msg = "You must set 'tax_benefit_system' to call this method." + raise ValueError(msg) + return self.tax_benefit_system.get_variable(variable_name, check_existence) def check_variable_defined_for_entity(self, variable_name: t.VariableName) -> None: """Check if ``variable_name`` is defined for ``self``. @@ -140,7 +135,7 @@ def check_variable_defined_for_entity(self, variable_name: t.VariableName) -> No >>> this = entities.SingleEntity("this", "", "", "") >>> that = entities.SingleEntity("that", "", "", "") >>> tax_benefit_system = taxbenefitsystems.TaxBenefitSystem([that]) - >>> this.set_tax_benefit_system(tax_benefit_system) + >>> this.tax_benefit_system = tax_benefit_system >>> this.check_variable_defined_for_entity("tax") Traceback (most recent call last): @@ -151,7 +146,7 @@ def check_variable_defined_for_entity(self, variable_name: t.VariableName) -> No ... value_type = int ... entity = that - >>> this._tax_benefit_system.add_variable(tax) + >>> this.tax_benefit_system.add_variable(tax) >>> this.check_variable_defined_for_entity("tax") @@ -160,7 +155,7 @@ def check_variable_defined_for_entity(self, variable_name: t.VariableName) -> No >>> tax.entity = this - >>> this._tax_benefit_system.update_variable(tax) + >>> this.tax_benefit_system.update_variable(tax) >>> this.check_variable_defined_for_entity("tax") diff --git a/openfisca_core/populations/_core_population.py b/openfisca_core/populations/_core_population.py index 0041a6927..ea4df54a3 100644 --- a/openfisca_core/populations/_core_population.py +++ b/openfisca_core/populations/_core_population.py @@ -87,7 +87,7 @@ def __call__( >>> population("salary", period) >>> tbs = taxbenefitsystems.TaxBenefitSystem([person]) - >>> person.set_tax_benefit_system(tbs) + >>> person.tax_benefit_system = tbs >>> simulation = simulations.Simulation(tbs, {person.key: population}) >>> population("salary", period) Traceback (most recent call last): @@ -365,7 +365,7 @@ def get_holder(self, variable_name: t.VariableName) -> t.Holder: ... value_type = int >>> tbs = taxbenefitsystems.TaxBenefitSystem([person]) - >>> person.set_tax_benefit_system(tbs) + >>> person.tax_benefit_system = tbs >>> population = populations.SinglePopulation(person) >>> simulation = simulations.Simulation(tbs, {person.key: population}) >>> population.get_holder("income_tax") diff --git a/openfisca_core/taxbenefitsystems/tax_benefit_system.py b/openfisca_core/taxbenefitsystems/tax_benefit_system.py index 1c582e240..509435aa4 100644 --- a/openfisca_core/taxbenefitsystems/tax_benefit_system.py +++ b/openfisca_core/taxbenefitsystems/tax_benefit_system.py @@ -72,7 +72,7 @@ def __init__(self, entities: Sequence[Entity]) -> None: entity for entity in self.entities if not entity.is_person ] for entity in self.entities: - entity.set_tax_benefit_system(self) + entity.tax_benefit_system = self @property def base_tax_benefit_system(self): @@ -572,7 +572,7 @@ def clone(self): ): new_dict[key] = value for entity in new_dict["entities"]: - entity.set_tax_benefit_system(new) + entity.tax_benefit_system = new new_dict["parameters"] = self.parameters.clone() new_dict["_parameters_at_instant_cache"] = {} diff --git a/openfisca_core/types.py b/openfisca_core/types.py index fe94b88d7..ceb8be0b8 100644 --- a/openfisca_core/types.py +++ b/openfisca_core/types.py @@ -262,6 +262,8 @@ def get_population(self, plural: None | str, /) -> CorePopulation: ... class TaxBenefitSystem(Protocol): person_entity: SingleEntity + @property + def variables(self, /) -> dict[VariableName, Variable]: ... def get_variable( self, variable_name: VariableName, diff --git a/tests/core/tools/test_runner/test_yaml_runner.py b/tests/core/tools/test_runner/test_yaml_runner.py index 6a02d14ce..6fa710578 100644 --- a/tests/core/tools/test_runner/test_yaml_runner.py +++ b/tests/core/tools/test_runner/test_yaml_runner.py @@ -15,7 +15,7 @@ class TaxBenefitSystem: def __init__(self) -> None: self.variables = {"salary": TestVariable()} self.person_entity = Entity("person", "persons", None, "") - self.person_entity.set_tax_benefit_system(self) + self.person_entity.tax_benefit_system = self def get_package_metadata(self): return {"name": "Test", "version": "Test"} From 3d57f6f355611c5532cc3fffdc346ed86b3e5a16 Mon Sep 17 00:00:00 2001 From: Mauko Quiroga Date: Fri, 25 Oct 2024 07:31:23 +0200 Subject: [PATCH 4/7] feat: add CoreEntity.variables --- openfisca_core/entities/_core_entity.py | 59 +++++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/openfisca_core/entities/_core_entity.py b/openfisca_core/entities/_core_entity.py index cb16450da..512705896 100644 --- a/openfisca_core/entities/_core_entity.py +++ b/openfisca_core/entities/_core_entity.py @@ -1,5 +1,6 @@ from __future__ import annotations +from collections.abc import Mapping from typing import ClassVar import abc @@ -52,6 +53,64 @@ def __init__(self, *__args: object, **__kwargs: object) -> None: ... def __repr__(self) -> str: return f"{self.__class__.__name__}({self.key})" + @property + def variables(self, /) -> Mapping[t.VariableName, t.Variable]: + """Get all variables defined for the entity. + + Returns: + dict[str, Variable]: The variables defined for the entity. + + Raises: + ValueError: When the :attr:`.tax_benefit_system` is not set yet. + + Examples: + >>> from openfisca_core import ( + ... entities, + ... periods, + ... taxbenefitsystems, + ... variables, + ... ) + + >>> this = entities.SingleEntity("this", "", "", "") + >>> that = entities.SingleEntity("that", "", "", "") + + >>> this.variables + Traceback (most recent call last): + ValueError: You must set 'tax_benefit_system' to call this method. + + >>> tbs = taxbenefitsystems.TaxBenefitSystem([this, that]) + >>> this, that = tbs.entities + + >>> this.variables + {} + + >>> that.variables + {} + + >>> class tax(variables.Variable): + ... definition_period = periods.MONTH + ... value_type = float + ... entity = that + + >>> tbs.add_variable(tax) + + + >>> this.variables + {} + + >>> that.variables + {'tax': } + + """ + if self.tax_benefit_system is None: + msg = "You must set 'tax_benefit_system' to call this method." + raise ValueError(msg) + return { + name: variable + for name, variable in self.tax_benefit_system.variables.items() + if variable.entity.key == self.key + } + def get_variable( self, variable_name: t.VariableName, From 1b2e08c9f6c130bcff99808bd904d13c4d738dd1 Mon Sep 17 00:00:00 2001 From: Mauko Quiroga Date: Fri, 25 Oct 2024 08:10:59 +0200 Subject: [PATCH 5/7] refactor: make CoreEntity.get_variable reuse .variables --- openfisca_core/entities/__init__.py | 3 ++ openfisca_core/entities/_core_entity.py | 49 ++++++++++++++----------- openfisca_core/entities/_errors.py | 32 ++++++++++++++++ 3 files changed, 63 insertions(+), 21 deletions(-) create mode 100644 openfisca_core/entities/_errors.py diff --git a/openfisca_core/entities/__init__.py b/openfisca_core/entities/__init__.py index fb00bc92f..32af3e0f0 100644 --- a/openfisca_core/entities/__init__.py +++ b/openfisca_core/entities/__init__.py @@ -2,6 +2,7 @@ from . import types from ._core_entity import CoreEntity +from ._errors import TaxBenefitSystemUnsetError, VariableNotFoundError from .entity import Entity from .group_entity import GroupEntity from .helpers import build_entity, find_role @@ -15,6 +16,8 @@ "GroupEntity", "Role", "SingleEntity", + "TaxBenefitSystemUnsetError", + "VariableNotFoundError", "build_entity", "find_role", "types", diff --git a/openfisca_core/entities/_core_entity.py b/openfisca_core/entities/_core_entity.py index 512705896..0ae969f89 100644 --- a/openfisca_core/entities/_core_entity.py +++ b/openfisca_core/entities/_core_entity.py @@ -7,6 +7,7 @@ import os from . import types as t +from ._errors import TaxBenefitSystemUnsetError, VariableNotFoundError class CoreEntity: @@ -61,7 +62,8 @@ def variables(self, /) -> Mapping[t.VariableName, t.Variable]: dict[str, Variable]: The variables defined for the entity. Raises: - ValueError: When the :attr:`.tax_benefit_system` is not set yet. + TaxBenefitSystemUnsetError: When the :attr:`.tax_benefit_system` is + not set yet. Examples: >>> from openfisca_core import ( @@ -71,12 +73,12 @@ def variables(self, /) -> Mapping[t.VariableName, t.Variable]: ... variables, ... ) - >>> this = entities.SingleEntity("this", "", "", "") - >>> that = entities.SingleEntity("that", "", "", "") + >>> this = entities.SingleEntity("this", "these", "", "") + >>> that = entities.SingleEntity("that", "those", "", "") >>> this.variables Traceback (most recent call last): - ValueError: You must set 'tax_benefit_system' to call this method. + TaxBenefitSystemUnsetError: The tax and benefit system is not se... >>> tbs = taxbenefitsystems.TaxBenefitSystem([this, that]) >>> this, that = tbs.entities @@ -103,8 +105,7 @@ def variables(self, /) -> Mapping[t.VariableName, t.Variable]: """ if self.tax_benefit_system is None: - msg = "You must set 'tax_benefit_system' to call this method." - raise ValueError(msg) + raise TaxBenefitSystemUnsetError return { name: variable for name, variable in self.tax_benefit_system.variables.items() @@ -115,7 +116,7 @@ def get_variable( self, variable_name: t.VariableName, check_existence: bool = False, - ) -> t.Variable | None: + ) -> None | t.Variable: """Get ``variable_name`` from ``variables``. Args: @@ -127,9 +128,10 @@ def get_variable( None: When the ``Variable`` doesn't exist. Raises: - ValueError: When the :attr:`_tax_benefit_system` is not set yet. - ValueError: When ``check_existence`` is ``True`` and - the ``Variable`` doesn't exist. + TaxBenefitSystemUnsetError: When the :attr:`.tax_benefit_system` is + not set yet. + VariableNotFoundError: When ``check_existence`` is ``True`` and the + ``Variable`` doesn't exist. Examples: >>> from openfisca_core import ( @@ -139,39 +141,44 @@ def get_variable( ... variables, ... ) - >>> this = entities.SingleEntity("this", "", "", "") - >>> that = entities.SingleEntity("that", "", "", "") + >>> this = entities.SingleEntity("this", "these", "", "") + >>> that = entities.SingleEntity("that", "those", "", "") >>> this.get_variable("tax") Traceback (most recent call last): - ValueError: You must set 'tax_benefit_system' before calling this... + TaxBenefitSystemUnsetError: The tax and benefit system is not se... - >>> tax_benefit_system = taxbenefitsystems.TaxBenefitSystem([this]) - >>> this.tax_benefit_system = tax_benefit_system + >>> tbs = taxbenefitsystems.TaxBenefitSystem([this, that]) + >>> this, that = tbs.entities >>> this.get_variable("tax") >>> this.get_variable("tax", check_existence=True) Traceback (most recent call last): - VariableNotFoundError: You tried to calculate or to set a value... + VariableNotFoundError: You requested the variable 'tax', but it ... >>> class tax(variables.Variable): ... definition_period = periods.MONTH ... value_type = float ... entity = that - >>> this.tax_benefit_system.add_variable(tax) + >>> tbs.add_variable(tax) >>> this.get_variable("tax") - + >>> that.get_variable("tax") + """ if self.tax_benefit_system is None: - msg = "You must set 'tax_benefit_system' to call this method." - raise ValueError(msg) - return self.tax_benefit_system.get_variable(variable_name, check_existence) + raise TaxBenefitSystemUnsetError + if not check_existence: + return self.variables.get(variable_name) + try: + return self.variables[variable_name] + except KeyError as error: + raise VariableNotFoundError(variable_name, self.plural) from error def check_variable_defined_for_entity(self, variable_name: t.VariableName) -> None: """Check if ``variable_name`` is defined for ``self``. diff --git a/openfisca_core/entities/_errors.py b/openfisca_core/entities/_errors.py new file mode 100644 index 000000000..dc0e586aa --- /dev/null +++ b/openfisca_core/entities/_errors.py @@ -0,0 +1,32 @@ +from . import types as t + + +class TaxBenefitSystemUnsetError(ValueError): + """Raised when a tax and benefit system is not set yet.""" + + def __init__(self) -> None: + msg = ( + "The tax and benefit system is not set yet. You need to set it " + "before using this method: `entity.tax_benefit_system = ...`." + ) + super().__init__(msg) + + +class VariableNotFoundError(IndexError): + """Raised when a requested variable is not defined for as entity.""" + + def __init__(self, name: t.VariableName, plural: t.EntityPlural) -> None: + msg = ( + f"You requested the variable '{name}', but it was not found in " + f"the entity '{plural}'. Are you sure you spelled '{name}' " + "correctly? If this code used to work and suddenly does not, " + "this is most probably linked to an update of the tax and " + "benefit system. Look at its CHANGELOG to learn about renames " + "and removals and update your code accordingly." + "Learn more about entities in our documentation:", + ".", + ) + super().__init__(msg) + + +__all__ = ["TaxBenefitSystemUnsetError", "VariableNotFoundError"] From ca92bdc4322ad6a75867187f2ef757d196f9d3d7 Mon Sep 17 00:00:00 2001 From: Mauko Quiroga Date: Fri, 25 Oct 2024 18:28:55 +0200 Subject: [PATCH 6/7] BREAKING CHANGE: remove CoreEntity.check_variable_defined_for_entity The feature is still provided by `CoreEntity.get_variable` when passed the optional positional or keyword argument `check_existence`. In fact, the removed method was duplicating and already existing behaviour. --- openfisca_core/entities/_core_entity.py | 85 ++----------------- openfisca_core/entities/_errors.py | 6 +- .../populations/_core_population.py | 4 +- .../simulations/simulation_builder.py | 4 +- openfisca_core/types.py | 26 +++--- tests/fixtures/entities.py | 12 +-- tests/web_api/test_calculate.py | 6 +- 7 files changed, 39 insertions(+), 104 deletions(-) diff --git a/openfisca_core/entities/_core_entity.py b/openfisca_core/entities/_core_entity.py index 0ae969f89..2fae0d167 100644 --- a/openfisca_core/entities/_core_entity.py +++ b/openfisca_core/entities/_core_entity.py @@ -4,7 +4,6 @@ from typing import ClassVar import abc -import os from . import types as t from ._errors import TaxBenefitSystemUnsetError, VariableNotFoundError @@ -103,6 +102,9 @@ def variables(self, /) -> Mapping[t.VariableName, t.Variable]: >>> that.variables {'tax': } + >>> that.variables["tax"] + + """ if self.tax_benefit_system is None: raise TaxBenefitSystemUnsetError @@ -115,6 +117,7 @@ def variables(self, /) -> Mapping[t.VariableName, t.Variable]: def get_variable( self, variable_name: t.VariableName, + /, check_existence: bool = False, ) -> None | t.Variable: """Get ``variable_name`` from ``variables``. @@ -173,81 +176,11 @@ def get_variable( """ if self.tax_benefit_system is None: raise TaxBenefitSystemUnsetError - if not check_existence: - return self.variables.get(variable_name) - try: - return self.variables[variable_name] - except KeyError as error: - raise VariableNotFoundError(variable_name, self.plural) from error - - def check_variable_defined_for_entity(self, variable_name: t.VariableName) -> None: - """Check if ``variable_name`` is defined for ``self``. - - Args: - variable_name: The ``Variable`` to be found. - - Raises: - ValueError: When the ``Variable`` exists but is defined - for another ``Entity``. - - Examples: - >>> from openfisca_core import ( - ... entities, - ... periods, - ... taxbenefitsystems, - ... variables, - ... ) - - >>> this = entities.SingleEntity("this", "", "", "") - >>> that = entities.SingleEntity("that", "", "", "") - >>> tax_benefit_system = taxbenefitsystems.TaxBenefitSystem([that]) - >>> this.tax_benefit_system = tax_benefit_system - - >>> this.check_variable_defined_for_entity("tax") - Traceback (most recent call last): - VariableNotFoundError: You tried to calculate or to set a value... - - >>> class tax(variables.Variable): - ... definition_period = periods.WEEK - ... value_type = int - ... entity = that - - >>> this.tax_benefit_system.add_variable(tax) - - - >>> this.check_variable_defined_for_entity("tax") - Traceback (most recent call last): - ValueError: You tried to compute the variable 'tax' for the enti... - - >>> tax.entity = this - - >>> this.tax_benefit_system.update_variable(tax) - - - >>> this.check_variable_defined_for_entity("tax") - - """ - entity: None | t.CoreEntity = None - variable: None | t.Variable = self.get_variable( - variable_name, - check_existence=True, - ) - - if variable is not None: - entity = variable.entity - - if entity is None: - return - - if entity.key != self.key: - message = ( - f"You tried to compute the variable '{variable_name}' for", - f"the entity '{self.plural}'; however the variable", - f"'{variable_name}' is defined for '{entity.plural}'.", - "Learn more about entities in our documentation:", - ".", - ) - raise ValueError(os.linesep.join(message)) + if (variable := self.variables.get(variable_name)) is not None: + return variable + if check_existence: + raise VariableNotFoundError(variable_name, self.plural) + return None __all__ = ["CoreEntity"] diff --git a/openfisca_core/entities/_errors.py b/openfisca_core/entities/_errors.py index dc0e586aa..fb223bf4d 100644 --- a/openfisca_core/entities/_errors.py +++ b/openfisca_core/entities/_errors.py @@ -12,7 +12,7 @@ def __init__(self) -> None: super().__init__(msg) -class VariableNotFoundError(IndexError): +class VariableNotFoundError(ValueError): """Raised when a requested variable is not defined for as entity.""" def __init__(self, name: t.VariableName, plural: t.EntityPlural) -> None: @@ -23,8 +23,8 @@ def __init__(self, name: t.VariableName, plural: t.EntityPlural) -> None: "this is most probably linked to an update of the tax and " "benefit system. Look at its CHANGELOG to learn about renames " "and removals and update your code accordingly." - "Learn more about entities in our documentation:", - ".", + "Learn more about entities in our documentation:" + "." ) super().__init__(msg) diff --git a/openfisca_core/populations/_core_population.py b/openfisca_core/populations/_core_population.py index ea4df54a3..048fbd9f8 100644 --- a/openfisca_core/populations/_core_population.py +++ b/openfisca_core/populations/_core_population.py @@ -142,7 +142,7 @@ def __call__( option=options, ) - self.entity.check_variable_defined_for_entity(calculate.variable) + self.entity.get_variable(calculate.variable, check_existence=True) self.check_period_validity(calculate.variable, calculate.period) if not isinstance(calculate.option, Sequence): @@ -380,7 +380,7 @@ def get_holder(self, variable_name: t.VariableName) -> t.Holder: None: for variable_name, variable_values in instance_object.items(): path_in_json = [entity.plural, instance_id, variable_name] try: - entity.check_variable_defined_for_entity(variable_name) + entity.get_variable(variable_name, check_existence=True) except ValueError as e: # The variable is defined for another entity raise errors.SituationParsingError(path_in_json, e.args[0]) except errors.VariableNotFoundError as e: # The variable doesn't exist @@ -665,6 +665,8 @@ def finalize_variables_init(self, population) -> None: for variable_name in self.input_buffer: try: holder = population.get_holder(variable_name) + # TODO(Mauko Quiroga-Alvarado): We should handle this differently. + # It cost me a lot of time to figure out that this was the problem. except ValueError: # Wrong entity, we can just ignore that continue buffer = self.input_buffer[variable_name] diff --git a/openfisca_core/types.py b/openfisca_core/types.py index ceb8be0b8..de0a62d62 100644 --- a/openfisca_core/types.py +++ b/openfisca_core/types.py @@ -1,6 +1,6 @@ from __future__ import annotations -from collections.abc import Iterable, Sequence, Sized +from collections.abc import Iterable, Mapping, Sequence, Sized from numpy.typing import DTypeLike, NDArray from typing import NewType, TypeVar, Union from typing_extensions import Protocol, Required, Self, TypeAlias, TypedDict @@ -71,19 +71,23 @@ class CoreEntity(Protocol): - key: EntityKey - plural: EntityPlural - - def check_variable_defined_for_entity( - self, - variable_name: VariableName, - /, - ) -> None: ... + @property + def key(self, /) -> EntityKey: ... + @property + def plural(self, /) -> EntityPlural: ... + @property + def label(self, /) -> str: ... + @property + def doc(self, /) -> str: ... + @property + def tax_benefit_system(self, /) -> None | TaxBenefitSystem: ... + @property + def variables(self, /) -> Mapping[VariableName, Variable]: ... def get_variable( self, - variable_name: VariableName, - check_existence: bool = ..., + name: VariableName, /, + check_existence: bool = ..., ) -> None | Variable: ... diff --git a/tests/fixtures/entities.py b/tests/fixtures/entities.py index 6670a68da..8f36b69e1 100644 --- a/tests/fixtures/entities.py +++ b/tests/fixtures/entities.py @@ -1,3 +1,5 @@ +from __future__ import annotations + import pytest from openfisca_core.entities import Entity, GroupEntity @@ -10,28 +12,22 @@ def get_variable( self, variable_name: str, check_existence: bool = False, - ) -> TestVariable: + ) -> None | TestVariable: result = TestVariable(self) result.name = variable_name return result - def check_variable_defined_for_entity(self, variable_name: str) -> bool: - return True - class TestGroupEntity(GroupEntity): def get_variable( self, variable_name: str, check_existence: bool = False, - ) -> TestVariable: + ) -> None | TestVariable: result = TestVariable(self) result.name = variable_name return result - def check_variable_defined_for_entity(self, variable_name: str) -> bool: - return True - @pytest.fixture def persons(): diff --git a/tests/web_api/test_calculate.py b/tests/web_api/test_calculate.py index f84642fb3..d672cee29 100644 --- a/tests/web_api/test_calculate.py +++ b/tests/web_api/test_calculate.py @@ -52,15 +52,15 @@ def check_response( ), ( '{"persons": {"bob": {"unknown_variable": {}}}}', - client.NOT_FOUND, + client.BAD_REQUEST, "persons/bob/unknown_variable", - "You tried to calculate or to set", + "You requested the variable 'unknown_variable', but it was not found in the entity 'persons'", ), ( '{"persons": {"bob": {"housing_allowance": {}}}}', client.BAD_REQUEST, "persons/bob/housing_allowance", - "You tried to compute the variable 'housing_allowance' for the entity 'persons'", + "You requested the variable 'housing_allowance', but it was not found in the entity 'persons'", ), ( '{"persons": {"bob": {"salary": 4000 }}}', From 1459e3110805b930e73a8bb54b4ed90c299ef759 Mon Sep 17 00:00:00 2001 From: Mauko Quiroga Date: Fri, 25 Oct 2024 20:42:44 +0200 Subject: [PATCH 7/7] chore: version bump --- CHANGELOG.md | 32 ++++++++++++++++++++++++++++++++ setup.py | 2 +- 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0f3b8eef0..0e4e32e5d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,37 @@ # Changelog +# 44.0.0 [#1286](https://github.com/openfisca/openfisca-core/pull/1286) + +#### New features + +- Add `CoreEntity.variables` + - Allows for querying the variables defined for an entity. + +#### Deprecations + +- Remove `CoreEntity.check_role_validity` + - The method in itself had no relation to `entities` (it was a static + method, so no depending neither on an entity class nor an instance). It + could have been kept as a `populations` helper, where it was exclusively + used. Nonetheless, `check_role_validity` had the peculiar behaviour of + not failing but when the value of `role` is not `None`, that granted it + in the author's opinion the right to be removed so as to avoid confusion. +- Deprecate `class_override` arg from `entities.build_entity` + - That argument actually did nothing, and linters have been complaining + about this argument for almost 10 years now. It is only being marked as + a breaking change for respect to the semantic versioning protocol. +- Deprecate `CoreEntity.check_variable_defined_for_entity` + - The feature is still provided by `CoreEntity.get_variable` when passed + the optional positional or keyword argument `check_existence`. In fact, + the removed method was duplicating and already existing behaviour. +- Deprecate `CoreEntity.set_tax_benefit_system` + - The feature is now provided by simple variable assignment: + ```python + entity.tax_benefit_system = tax_benefit_system + ``` + - Likewise, `entity.tax_benefit_system` becomes part of the public API + (which implicitly it was as the private marker was being violated). + ### 43.2.5 [#1296](https://github.com/openfisca/openfisca-core/pull/1296) #### Bugfix diff --git a/setup.py b/setup.py index b466b2407..80f45041d 100644 --- a/setup.py +++ b/setup.py @@ -70,7 +70,7 @@ setup( name="OpenFisca-Core", - version="43.2.5", + version="44.0.0", author="OpenFisca Team", author_email="contact@openfisca.org", classifiers=[