test: encapsulate periods module #1138

bonjourmauko · 2022-07-28T19:48:35Z

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

CHANGELOG.md

openfisca_core/periods/instant_.py

openfisca_core/periods/period_.py

setup.cfg

openfisca_tasks/test_code.mk

MattiSG · 2022-07-29T17:41:33Z

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 · 2022-07-30T08:48:03Z

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 · 2022-07-31T10:00:31Z

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 · 2022-08-01T11:31:07Z

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 · 2022-08-02T11:00:48Z

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

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

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

MattiSG · 2022-12-19T14:08:43Z

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 · 2022-12-19T23:29:04Z

openfisca_core/periods/period_.py

+            f"{str(self.unit)}."
+            )
+
+    def this(self, unit: DateUnit) -> Period:


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

bonjourmauko · 2022-12-21T07:32:32Z

@eraviart What do you think?

bonjourmauko mentioned this pull request Jul 28, 2022

Improve package's doctests #1061

Open

17 tasks

bonjourmauko requested review from a team July 28, 2022 19:53

bonjourmauko force-pushed the doctests/doctests-standalone branch from 804a3cd to 24cafc0 Compare July 29, 2022 01:44

MattiSG reviewed Jul 29, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

openfisca_core/periods/instant_.py Show resolved Hide resolved

openfisca_core/periods/period_.py Show resolved Hide resolved

setup.cfg Outdated Show resolved Hide resolved

openfisca_tasks/test_code.mk Show resolved Hide resolved

bonjourmauko force-pushed the doctests/doctests-standalone branch 2 times, most recently from 583cedd to 57b74ca Compare July 31, 2022 09:51

bonjourmauko requested a review from MattiSG July 31, 2022 09:53

This was referenced Jul 31, 2022

Add date units to periods #1139

Closed

Documentation testing broken since flake8 upgrade #1140

Closed

bonjourmauko force-pushed the doctests/doctests-standalone branch from 22457c5 to 9d1c0c8 Compare July 31, 2022 13:58

bonjourmauko added the kind:refactor Refactoring and code cleanup label Jul 31, 2022

bonjourmauko force-pushed the doctests/doctests-standalone branch 2 times, most recently from a12d131 to bc9e1aa Compare July 31, 2022 19:23

MattiSG reviewed Aug 1, 2022

View reviewed changes

bonjourmauko requested a review from MattiSG August 1, 2022 19:02

bonjourmauko force-pushed the doctests/doctests-standalone branch from 80c4880 to 3a36901 Compare November 21, 2022 10:59

bonjourmauko requested a review from a team November 21, 2022 11:00

openfisca deleted a comment from coveralls Dec 1, 2022

bonjourmauko force-pushed the doctests/doctests-standalone branch from 3a36901 to 1dc83c9 Compare December 1, 2022 17:20

bonjourmauko added 5 commits December 13, 2022 23:41

Fix flake8/pycodestyle dependency error

9872a39

Add periods to doctests path

35e02c5

Ignore mypy periods' tests errors

8ba7fe1

Fix instants' docstrings

496951f

Fix periods' doctests

fbafc60

bonjourmauko added 12 commits December 18, 2022 12:04

Use named isoformat attrs

fc644f7

Simplify iso parser

cbcceab

Fix type error in instant

f84afbf

Update CHANGELOG

97de3ee

Cleanup types

2fc824e

Add more examples to DateUnit

499d894

Remove unused exception

6addf76

Make simpler test execution

f755138

Get rid of last type stub

574973c

Minimise changes to the public API

a6c00c9

Reuse parsers to simplify period build

dd523b6

Merge branch 'master' into doctests/doctests-standalone

bae8d52

bonjourmauko added 3 commits December 20, 2022 00:19

Improved period factory

091fe10

Improve packaging

7e1d3b3

Keep file names

23facd0

bonjourmauko commented Dec 19, 2022

View reviewed changes

bonjourmauko mentioned this pull request Dec 19, 2022

Add importlib_metadata requirement #1177

Closed

bonjourmauko mentioned this pull request Dec 22, 2022

Remplace les méthodes .period par une instanciation de la classe Period() openfisca/openfisca-france#1961

Open

bonjourmauko added 8 commits December 22, 2022 19:28

Add the offsetable protocol

d82f78f

Make Instant more anglo-saxpon friendly

aac33e1

Fix period implementation

7e17e58

Fix offsetable type

574cf5f

Make Instant._add private

4c168df

Refactor Instant.offset

d3b24df

Make Period.__str__ less surprising

30e4164

Harmonise offset syntax

56daa66

bonjourmauko marked this pull request as draft October 1, 2024 02:55

bonjourmauko changed the title ~~[7/17] Encapsulate periods module~~ test: encapsulate periods module Oct 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: encapsulate periods module #1138

test: encapsulate periods module #1138

bonjourmauko commented Jul 28, 2022 •

edited

Loading

MattiSG left a comment

MattiSG commented Jul 29, 2022

bonjourmauko commented Jul 30, 2022

bonjourmauko commented Jul 31, 2022

MattiSG left a comment

bonjourmauko commented Aug 1, 2022

bonjourmauko commented Aug 2, 2022

MattiSG commented Dec 19, 2022

bonjourmauko Dec 19, 2022

bonjourmauko Dec 21, 2022

bonjourmauko commented Dec 21, 2022

test: encapsulate periods module #1138

Are you sure you want to change the base?

test: encapsulate periods module #1138

Conversation

bonjourmauko commented Jul 28, 2022 • edited Loading

Breaking changes

Renames

Deprecations

Structural changes

Technical changes

Bug fixes

MattiSG left a comment

Choose a reason for hiding this comment

MattiSG commented Jul 29, 2022

bonjourmauko commented Jul 30, 2022

bonjourmauko commented Jul 31, 2022

MattiSG left a comment

Choose a reason for hiding this comment

bonjourmauko commented Aug 1, 2022

bonjourmauko commented Aug 2, 2022

MattiSG commented Dec 19, 2022

bonjourmauko Dec 19, 2022

Choose a reason for hiding this comment

bonjourmauko Dec 21, 2022

Choose a reason for hiding this comment

bonjourmauko commented Dec 21, 2022

bonjourmauko commented Jul 28, 2022 •

edited

Loading