Skip to content

Re-add support for dispatch/divide by days#1143

Merged
bonjourmauko merged 5 commits intomasterfrom
fix-day-periods
Nov 21, 2022
Merged

Re-add support for dispatch/divide by days#1143
bonjourmauko merged 5 commits intomasterfrom
fix-day-periods

Conversation

@bonjourmauko
Copy link
Copy Markdown
Member

@bonjourmauko bonjourmauko commented Jul 31, 2022

Fixes digitalaotearoa/openfisca-aotearoa#15
Depends on #1141

New features

  • Introduce support for the day date unit in holders.set_input_dispatch_by_period and holders.set_input_divide_by_period
    • Allows for dispatching values per day, for example, to provide a daily (week, fortnight) to an yearly variable.
    • Inversely, allows for calculating the daily (week, fortnight) value of a yearly input.

@bonjourmauko bonjourmauko requested review from a team July 31, 2022 19:15
@bonjourmauko bonjourmauko added the kind:feat A feature request, a feature deprecation label Jul 31, 2022
@MattiSG
Copy link
Copy Markdown
Member

MattiSG commented Aug 1, 2022

I will postpone reviewing this PR until the one it depends on is approved, and suggest that this one is set to draft. Indeed, I fail to see how it could be approved when #1141 might not go through.

Do you truly need #1141 for this one to be reviewed? 🙂 As an alternative to switching to draft, I suggest to remove the dependency and rebase straight on master. Yes, test-docs might fail until #1141 is fixed, but that is true of all PRs, not only of this one. This would enable reviews to be parallelised and proper solutions found for #1140 without blocking all the others.

@bonjourmauko
Copy link
Copy Markdown
Member Author

Done!

@bonjourmauko
Copy link
Copy Markdown
Member Author

I'm not sure if this is a new feature, or a bug-fix as the behaviour this adds was introduced as a replacement of a behaviour that was deprecated, however the pull request forgot to include days in that migration.

@benjello
Copy link
Copy Markdown
Member

@maukoquiroga : looks very good to me.
Just a question: how does this PR handles month day size, february and leeap year for divide_by_period ?
It does not seem to be tested although there are a bunch of tests for that in Period and Instant.

@bonjourmauko
Copy link
Copy Markdown
Member Author

@maukoquiroga : looks very good to me. Just a question: how does this PR handles month day size, february and leeap year for divide_by_period ? It does not seem to be tested although there are a bunch of tests for that in Period and Instant.

I didn't do a lot more than «reenabling» a feature that already existed.

If you see in the tests, month days are handled appropriately (31 + 28 + 30).

Maybe test should be more precise about leap years.

@bonjourmauko
Copy link
Copy Markdown
Member Author

(By the way, I think that holders implements it's own logic in terms of equivalences between days, months and years. We should see which is better, and keep just one in periods, as holder should not handle that kind of logic IMHO).

@openfisca openfisca deleted a comment from coveralls Nov 17, 2022
@bonjourmauko
Copy link
Copy Markdown
Member Author

@benjello @MattiSG could you please take a look at this pull request again?

@bonjourmauko
Copy link
Copy Markdown
Member Author

cc @MattiSG @benjello

@bonjourmauko bonjourmauko changed the title Add support for dispatch/divide by days Readd support for dispatch/divide by days Nov 21, 2022
@bonjourmauko bonjourmauko changed the title Readd support for dispatch/divide by days Re-add support for dispatch/divide by days Nov 21, 2022
@bonjourmauko bonjourmauko merged commit ea57e8e into master Nov 21, 2022
@bonjourmauko bonjourmauko deleted the fix-day-periods branch November 21, 2022 09:52
@bonjourmauko bonjourmauko added this to the Add support for weeks milestone Sep 17, 2024
@bonjourmauko bonjourmauko mentioned this pull request Sep 17, 2024
@bonjourmauko bonjourmauko mentioned this pull request Sep 25, 2024
32 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind:feat A feature request, a feature deprecation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Tests removed while OpenFisca Core upgrade required

3 participants