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

BREAKING CHANGE: cleanup entities #1286

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

bonjourmauko
Copy link
Member

@bonjourmauko bonjourmauko commented Oct 25, 2024

Depends on #1285

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

Technical changes

  • Fix failing openfisca-extension-template tests

@bonjourmauko bonjourmauko added the kind:refactor Refactoring and code cleanup label Oct 25, 2024
@bonjourmauko bonjourmauko requested review from a team October 25, 2024 18:40
@bonjourmauko bonjourmauko self-assigned this Oct 25, 2024
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.
…ity`

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.
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).
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.
@benjello
Copy link
Member

@bonjourmauko : please take a look at openfisca-survey-manager to see if these deprecations do affect the package.
openfisca-core and openfisca-survey-manager are closely related through usage.
Indeed, a lot of users use country packages with openfisca-survey-manager and may end-up in trouble if their country package works needs the latest version of openfisca-core and while having recent legislation but cannot run with
openfisca-survey-manager.
IMHO, the version bumps should follow openfisca-core -> openfisca-survey-manager -> openfisca-country
And the sooner we check, the best we are prepared.

cc @sandcha @benoit-cty @clallemand @sylvainipp

@bonjourmauko
Copy link
Member Author

We can temporise this one, it is not urgent (it is purely deprecations, so it is expected that it wreaks havoc down the line; yet all deprecations are duplicates with corresponding features).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:refactor Refactoring and code cleanup
Projects
Development

Successfully merging this pull request may close these issues.

2 participants