[7/17] Encapsulate periods module

[bonjourmauko]

Part of #1061 Part of #1062 Part of #1063 Supersedes #1139 Supersedes #1043 Depended upon by #1146

Breaking changes

Renames

• Rename periods.period.get_subperiods to periods.period.subperiods.

Deprecations

• Deprecate INSTANT_PATTERN
  • The feature is now provided by periods.isoformat.fromstr
• Deprecate instant_date.
  • The feature is now provided by periods.instant.date.
• Deprecate periods.{unit_weight, unit_weights, key_period_size}.
  • These features are now provided by periods.dateunit.
• Deprecate periods.intersect.
  • The feature has no replacement.
• Make periods.parse_period stricter.
  • For example 2022-1 now fails.
• Refactor periods.period.contains as __contains__.
  • For example subperiod in period is now possible.

Structural changes

• Transform Period.date from property to method.
  • Now it has to be used as period.date() (note the parenthesis).
• Transform Instant.date from property to method.
  • Now it has to be used as instant.date() (note the parenthesis).
• Rationalise the reference periods.
  • Before, there was a definite list of reference periods. For example, period.first_month or period.n_2.
  • This has been simplified to allow users to build their own:
  • period.ago(unit: DateUnit, size: int = 1) -> Period.
  • period.last(unit: DateUnit, size: int = 1) -> Period.
  • period.this(unit: DateUnit) -> Period.
• Rationalise date units.
  • Before, usage of "month", YEAR, and so on was fairly inconsistent, and providing a perfect hotbed for bugs to breed.
  • This has been fixed by introducing a new dateunit module, which provides a single source of truth for all date units.
  • Note that if you used periods.YEAR and the like, there is nothing to change in your code.
  • However, strings like "year" or "ETERNITY" are no longer allowed (in fact, date unit are int enums an no longer strings).

Technical changes

• Add typing to openfisca_core.periods.
• Fix openfisca_core.periods doctests.
• Document openfisca_core.periods.

Bug fixes

• Fixes incoherent dates.
• Fixes several race conditions.

[MattiSG]

In order to ease future reviews, when you open a PR that depends on another one, could you please make it explicit in the description? 🙂 This will make it easier to review 😊

[bonjourmauko]

Thank you for these contributions! I still don't understand how to validate doctests, and I suffered a lot from trying to set them straight (cf. #1134). I see here that we in the end ignore errors for the “periods” module. If that is indeed the case, then could you help me understand what is the point of “fixing” those tests? 🙂

Hi. Actually this PR make periods' doctest run (that's how I fixed them), so for them to be part of the test suite. What is ignored, at least for now, are tests' type checks, that's it.

Doctests are valuable in that they're the rare expression of unit tests in this library, coming from the contributors who gave birth to much the codebase.

[bonjourmauko]

Hello @MattiSG ! I hope to have addressed all of your concerns. I have also updated the description to make it clear that this PR only addresses "tests", including those in the docstrings (doctests).

[bonjourmauko]

I cannot review this as it depends on #1141 that is disputed, and I don't support changing so many dependencies in an untestable way (cf. #1140 (comment)).

Understood.

In order to ease reviews, could you please avoid force pushing once the PR has been opened, and rely instead on fixup commits if necessary? 🙂 Otherwise, I cannot rely on GitHub’s presentation of what has “changed since last review”, forcing me to spend time reviewing the same changeset again.

OK.

[bonjourmauko]

@MattiSG I've had cleanup as much as I can.

IMHO both readability and test coverage have improved, which is a triple win:

1. We know several functions to deprecate soon.
2. We can now more safely extend periods! :) (I've found some bugs downstream already).
3. I forgot this one (something related to types, contracts, and maintainability).

[bonjourmauko]

Hmm I did a git mv but it still marked it as deleted.

[bonjourmauko]

It might be worthwhile to run openfisca-france using this PR to fix here and there the impacts on a large "prod" repo.

@benjello This pull request doesn't add any feature but deprecates some and fixes several bugs. In general, it renders the whole module more reliable, as many edge-cases that went silent before will now yield a useful error. Yet, of course, that means breaking changes as the range of the acceptable shrunk. It's a fair trade-off and a much needed contribution, but requires thorough testing. I'd be ideal to run this against France with population data 🏛️

[benjello]

@maukoquiroga : as is it breaks openfisca-france test because period method is deprecated and replaced by build_period so it might be efficient to migrate the changes in openfisca-france concomitantly.

What do you think @sandcha @MattiSG ?

[bonjourmauko]

@maukoquiroga : as is it breaks openfisca-france test because period method is deprecated and replaced by build_period so it might be efficient to migrate the changes in openfisca-france concomitantly.

What do you think @sandcha @MattiSG ?

The rename is a find & replace, I'm more wondering if the stringer contracts will break parts of the model. For example, some cases returning None now fail, invalid periods like 2022-1-1 also fail, etc.

Would this be suitable @MattiSG for your proposal of a release candidate?

[MattiSG]

Would this be suitable @MattiSG for your proposal of a release candidate?

Yes, of course, this is a good case for #1159. But I want to stress once more that #1159 is not about allowing the use of release candidates, but about allowing replacing code review by extensive testing in some contexts. In the given case, I understand that @benjello says testing on France would be welcome. If you want to simplify that testing by using a release candidate (as opposed to a local clone), sure, do it! No need for #1159 for that 😉

[bonjourmauko]

@eraviart What do you think?