test: encapsulate periods module

[bonjourmauko]

Fixes #1269
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]

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? 🙂

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

[MattiSG]

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

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.

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

[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]

@benjello look from here below, what do you think?

[bonjourmauko]

@eraviart

[bonjourmauko]

@eraviart What do you think?