fava icon indicating copy to clipboard operation
fava copied to clipboard

Unconsistency in week Date filter

Open mailhot001 opened this issue 1 year ago • 6 comments

The week filter is not consistent, there is a 1 week offset when appling a date filter. For example if you rund the DEMO.

Show the Expenses, and apply the daily interval, with the changes(weekly) display, the expenses are shown per week, as expected.

image

If you click on the 2017-W36 week, there will be no data shown, to show the 2017-W36 expenses you need to filter the date to 2017-W35.

I believe the proper week is 2017-W36 so there is a -1 week difference in the week date filter?

mailhot001 avatar May 13 '24 15:05 mailhot001

In fava/src/fava/util/date.py there is the following function: image There is a +1 in the date defining, so if a date is ISO week no. 1 it will return week 2. So it seem to be a wanted behavious, but fava on the display use the standard ISO week format, thus the inconsistency?

mailhot001 avatar May 16 '24 16:05 mailhot001

Test did not pass, confirming this is a wanted behaviour, so maybe we should change the week number display on weekly expenses display? image

mailhot001 avatar May 16 '24 16:05 mailhot001

Test did not pass, confirming this is a wanted behaviour

That's not necessarily the case - tests are like code, they have bugs too ;) The wanted behaviour here would be for frontend and backend to match up of course. IMHO both should just match what strftime does for %W (here's how the Python docs describe that: "Week number of the year (Monday as the first day of the week) as a zero-padded decimal number. All days in a new year preceding the first Monday are considered to be in week 0.".) - the frontend does that using d3-time-format, we should change the Python side to match.

yagebu avatar May 18 '24 08:05 yagebu

Thanks for the info, I will double check the d3 to confirm it does the same as python fromisocalendar regarding week handling and summit an updated pull request afterwards.

mailhot001 avatar May 21 '24 13:05 mailhot001

You can have a look at the updated #1807 let me know what you think. I believe the python week and d3 would align perfectly with this.

Side note, in general those who cares about week numbers are mostly european using ISO format for week number, in america we rarely use week number. You can have a read with this here it makes a lot of sense. So I wonder if eventually switching week numbering to ISO could be a better solution?

mailhot001 avatar Jun 19 '24 13:06 mailhot001

Thanks for the fix, I've merged your PR.

Side note, in general those who cares about week numbers are mostly european using ISO format for week number, in america we rarely use week number. You can have a read with this here it makes a lot of sense. So I wonder if eventually switching week numbering to ISO could be a better solution?

I agree that ISO week numbers would probably be a better choice (I personally rarely use week numbers so it doesn't make much a difference to me)

yagebu avatar Jun 22 '24 15:06 yagebu