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.

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

feat: add fortnightly interval #1955

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

JP-Ellis
Copy link

@JP-Ellis JP-Ellis commented Feb 4, 2025

In some countries (such as Australia), it is very common for wages to be paid on a fortnightly basis, that is, every 2 weeks. As a result, a number of other expenses are also paid on a fortnightly basis.

This commit extends the Interval enum by adding a FORTNIGHT option. I have added a test for that, and have updated the translations where possible (not that other than for English and French, the other translations may be suboptimal).

As part of this commit, I have also taken the opportunity to fix a confusion between ISO weeks and calendar weeks as the two do not always match, and 2025-W01 is the ISO format indicating the week starting on December 30 2024. Let me know if you want me to split out this particular change into a separate PR. EDIT: I have moved this into #1956.

Resolves: #1939

@JP-Ellis JP-Ellis force-pushed the feat/fortnight-interval branch 4 times, most recently from 6fba497 to 4a34f47 Compare February 5, 2025 07:55
Copy link
Member

@yagebu yagebu left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, looks good - I just left some minor comments.

@@ -93,6 +96,31 @@ class Interval(Enum):
YEAR = "year"
QUARTER = "quarter"
MONTH = "month"
FORTNIGHT = "fortnight"
# Note: While a fortnight is a well-defined time interval, there are no
Copy link
Member

Choose a reason for hiding this comment

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

Can you move this (maybe in a slightly shortened form) to the help pages this seems relevant for users to understand how this works

# If we are in the first fortnight of the year, finding the last
# fortnight of the previous year is a bit tricky due to being either
# starting on week 51 or 53 (if the year has 53 weeks).
try:
Copy link
Member

Choose a reason for hiding this comment

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

This file had complete code coverage in tests (except for the annotated lines), with your changes some cases don't seem to be fully covered - e.g. this try-except-block. Can you add some more tests for those? (make test will show the lines or you can take a look at the HTML coverage in htmlcov)

@@ -1,28 +1,36 @@
import { _ } from "../i18n";

export type Interval = "year" | "quarter" | "month" | "week" | "day";
export type Interval =
| "year"
Copy link
Member

Choose a reason for hiding this comment

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

seems these lines are indented with tabs and not spaces - I noticed ts was missing from the pre-commit config for prettier, I just fixed that on main - can you rebase and re-run pre-commit to switch to spaces here as well?

Copy link
Author

Choose a reason for hiding this comment

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

I noticed the formatting changes and just assumed it was covered pre-commit (maybe incrementally changes the style or something)

More than happy to rebase 👍

In some countries (such as Australia), it is very common for wages to be
paid on a fortnightly basis, that is, every 2 weeks. As a result, a
number of other expenses are also paid on a fortnightly basis.

This commit extends the `Interval` enum by adding a `FORTNIGHT` option.
I have added a test for that, and have updated the translations where
possible (not that other than for English and French, the other
translations may be suboptimal).

Resolves: beancount#1939
Signed-off-by: JP-Ellis <[email protected]>
Made a few improvements to the budgets docs by:

- Specifying how budgets can replace previous one using an example
- Expanding a bit on how each interval is calculated
- Some minor refactors to help everything flow a bit better

Signed-off-by: JP-Ellis <[email protected]>
@JP-Ellis JP-Ellis force-pushed the feat/fortnight-interval branch from 4a34f47 to 84fcdbb Compare February 8, 2025 23:43
I also realised I misunderstood the 'get_prev_interval' which returns
that start of the current interval. So good to have improved test
coverage.

Signed-off-by: JP-Ellis <[email protected]>
@JP-Ellis
Copy link
Author

JP-Ellis commented Feb 9, 2025

I have updated the tests to ensure 100% test coverage. I did have to add to lines with # pragma nocover because they should be unreachable.

For the coverage, would you be open to a PR adding integration with Codecov? This will help see coverage of PRs automatically, and it is free for OSS projects. You can see an example of how Codecov integrates within GitHub at:

pact-foundation/pact-python#935

And you can follow links therein to see the more detailed coverage reports.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a 'fortnightly' frequency for budgets
2 participants