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.

Sign up for GitHub

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

Jump to bottom

refactor: add typing to & do maintenance of periods #1223

Merged
merged 20 commits into from
Sep 30, 2024
Merged

refactor: add typing to & do maintenance of periods #1223

merged 20 commits into from
Sep 30, 2024

Conversation

bonjourmauko
Copy link
Member

@bonjourmauko bonjourmauko commented Sep 17, 2024 •
edited
Loading

Fixes #917
Fixes #1229
Fixes #1232
Depends on #1220
Depended upon by #1146

Breaking changes

  • Changes to eternity instants and periods
    • Eternity instants are now <Instant(-1, -1, -1)> instead of
      <Instant(inf, inf, inf)>
    • Eternity periods are now <Period(('eternity', <Instant(-1, -1, -1)>, -1))>
      instead of <Period(('eternity', <Instant(inf, inf, inf)>, inf))>
    • The reason is to avoid mixing data types: inf is a float, periods and
      instants are integers. Mixed data types make memory optimisations impossible.
    • Migration should be straightforward. If you have a test that checks for
      inf, you should update it to check for -1 or use the is_eternal method.
  • periods.instant no longer returns None
    • Now, it raises periods.InstantError

New features

  • Introduce Instant.eternity()
    • This behaviour was duplicated across
    • Now it is encapsulated in a single method
  • Introduce Instant.is_eternal and Period.is_eternal
    • These methods check if the instant or period are eternity (bool).
  • Now periods.instant parses also ISO calendar strings (weeks)
    • For instance, 2022-W01 is now a valid input

Technical changes

  • Update pendulum
  • Reduce code complexity
  • Remove run-time type-checks
  • Add typing to the periods module

@bonjourmauko bonjourmauko added level:intermediate Requires average OpenFisca experience kind:refactor Refactoring and code cleanup labels Sep 17, 2024
@bonjourmauko bonjourmauko self-assigned this Sep 17, 2024
@coveralls
Copy link

coveralls commented Sep 17, 2024 •
edited
Loading

Coverage Status

coverage: 74.551% (+0.06%) from 74.491%
when pulling e68657e on fix-mypy-checks-periods
into 7d451a1 on master.

@bonjourmauko bonjourmauko requested review from a team September 17, 2024 22:35
@bonjourmauko bonjourmauko mentioned this pull request Sep 17, 2024
Draft
@bonjourmauko bonjourmauko changed the base branch from master to fix-mypy-checks-ent September 17, 2024 22:37
Base automatically changed from fix-mypy-checks-ent to master September 18, 2024 11:09
@bonjourmauko bonjourmauko mentioned this pull request Sep 18, 2024
Open
17 tasks
@bonjourmauko bonjourmauko force-pushed the fix-mypy-checks-periods branch from 3601736 to 9317b7f Compare September 18, 2024 11:47
@bonjourmauko bonjourmauko changed the title [9/17] Typing & maintenance to periods [7/17] Typing & maintenance to periods Sep 19, 2024
@bonjourmauko bonjourmauko mentioned this pull request Sep 19, 2024
Merged
@bonjourmauko bonjourmauko force-pushed the fix-mypy-checks-periods branch from 9317b7f to eb159e8 Compare September 19, 2024 11:56
@bonjourmauko bonjourmauko force-pushed the fix-mypy-checks-periods branch from d2659c6 to b27db7c Compare September 24, 2024 18:20
benjello
benjello requested changes Sep 25, 2024
View reviewed changes
Copy link
Member

@benjello benjello left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an empty file openfisca_core/periods/py.typed

You should check these changes against openfisca-france and openfisca-survey-manager since it may surface some problems that should be documented in the changelog to ease transition and to diagnose the right version bump.

openfisca_core/periods/_parsers.py Outdated Show resolved Hide resolved
openfisca_core/periods/helpers.py Outdated Show resolved Hide resolved
openfisca_core/periods/instant_.py Show resolved Hide resolved
@bonjourmauko bonjourmauko changed the title [7/17] Typing & maintenance to periods refactor: add typing to & do maintenance of periods Sep 25, 2024
This was referenced Sep 25, 2024
Draft
Open
@bonjourmauko
docs(periods): fix types (#1229)
dd4ca9f
@bonjourmauko
style(periods): fix linter errors (#1229)
93eec6c
@bonjourmauko
refactor(periods)!: fix instant complexity (#1229)
1ef1d91 
Method `periods.instant` had way too much branching and cyclomatic
complexity. It has been refactored so to make use of
`functools.singledispatch` to improve readability and testing.

BREAKING CHANGE: `periods.instant` no longer returns `None`. Now, it
raises `periods.InstantError` instead.
@bonjourmauko
refactor(periods): ensure is str in parse (#1229)
33f00cf 
Broaden the expected argument type to be of generic type `object`, and
do an explicit casting to help users know that the expected argument for
that function is a string.
@bonjourmauko
style(periods): fix linter errors (#1229)
d5ef321
@bonjourmauko
feat(periods): parse week instants (#1232)
022e2c4 
fixes #1232
@bonjourmauko
refactor(periods)!: change eternity to int (#1229)
e1aa7c7 
Previously, eternal periods and entities were populated with `inf`
values, which are floats. This was an exception, already, as the rest of
the values are integers. If you store, for example, thousands of
instants in a numpy array, just one eternal instant will force the whole
array to pass from integer to float. Now, eternal instants are populated
with `-1`, and can be produced with `Instant.eternity()` and
`Period.eternity()`. Also, they can be checked with `is_eternal`.
@bonjourmauko
chore(version): bump
e68657e
@bonjourmauko
Copy link
Member Author

There is an empty file openfisca_core/periods/py.typed

That is pure documentation. Is to let others know the code is typed. See: https://peps.python.org/pep-0561/

You should check these changes against openfisca-france and openfisca-survey-manager since it may surface some problems that should be documented in the changelog to ease transition and to diagnose the right version bump.

OK.

@bonjourmauko bonjourmauko mentioned this pull request Sep 26, 2024
Merged
7 tasks
This was referenced Sep 26, 2024
Merged
Merged
@bonjourmauko
Copy link
Member Author

There is an empty file openfisca_core/periods/py.typed

You should check these changes against openfisca-france and openfisca-survey-manager since it may surface some problems that should be documented in the changelog to ease transition and to diagnose the right version bump.

It all works @benjello :)

In France, the only check that fails is the Conda build, but that is because we're testing against a URL and not a PyPi release, which is normal, as this has yet to be merged and released.

The important is that the tests pass.

@bonjourmauko
Copy link
Member Author

LGTM?

@bonjourmauko bonjourmauko merged commit adcc161 into master Sep 30, 2024
22 checks passed
@bonjourmauko bonjourmauko deleted the fix-mypy-checks-periods branch September 30, 2024 19:52
bonjourmauko added a commit to openfisca/country-template that referenced this pull request Sep 30, 2024
@bonjourmauko
chore: update to core 42.0.0 (#154)
db249b7 
Depends on openfisca/openfisca-core#1223

* Minor change.
  - Update OpenFisca-Core to 42.0.0
@bonjourmauko bonjourmauko mentioned this pull request Oct 4, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benjello benjello benjello approved these changes

@benoit-cty benoit-cty Awaiting requested review from benoit-cty

Assignees

@bonjourmauko bonjourmauko

Labels
kind:refactor Refactoring and code cleanup level:intermediate Requires average OpenFisca experience
Projects
OpenFisca International
Status: Shipped
Milestone
Improved doc, style & linting
3 participants
@bonjourmauko @coveralls @benjello