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

Update end-dates in comments corresponding to PR #2522 #47

Merged
merged 1 commit into from
Jul 27, 2021
Merged

Update end-dates in comments corresponding to PR #2522 #47

merged 1 commit into from
Jul 27, 2021

Conversation

smemsh
Copy link
Contributor

@smemsh smemsh commented Jul 19, 2021

make the comments accurate after changes, cf issue #2519

updates end-dates in comments for eod, eom, eoy, etc

cf issue #2519
@smemsh
Copy link
Contributor Author

smemsh commented Jul 19, 2021

ran the following script to get these, on current 2.6.0 head:

for datename in $(
    grep ^//...eo src/Datetime.cpp |
    awk '{print $2}'
); do
    echo -n "$datename "
    faketime '2017-03-05 12:34:56' task calc $datename
done | column -t

The diff looks a bit strange, some things changed by more than I expected them to. Let me look at it again to make sure it's right, standby.

@smemsh
Copy link
Contributor Author

smemsh commented Jul 19, 2021

yes, I think they are all correct, but I'm a little confused about these:

eopww  2017-03-03T23:59:59
eoww   2017-03-10T23:59:59
eonww  2017-03-17T23:59:59

with the reference date of 2017-03-05 12:34:56 (a Sunday), and the calendar:

 $ ncal 03 2017
    March 2017        
Mo     6 13 20 27   
Tu     7 14 21 28   
We  1  8 15 22 29   
Th  2  9 16 23 30   
Fr  3 10 17 24 31   
Sa  4 11 18 25      
Su  5 12 19 26      
    9 10 11 12 13   

do these look right? It seems to consider the next "work week" to have already begun on Saturday, but I would say the next work week doesn't actually begin until Monday. This would be a separate bug; comment updates in this patch appear to be accurate by manual compare to calendar, assuming the work week calculation is correct. I just think it's strange to say that the work week already cycled to the next one after midnight on Friday, it shouldn't actually change to the next one until Monday 00:00:00 (imo). On Saturday and Sunday it should still think "this work week" is the one that ended the previous Friday, no?

That aside, the patch could be merged as-is and would reflect the current code.

@tbabej tbabej added the todo There is an action point associated with the issue/PR. label Jul 27, 2021
@tbabej
Copy link
Member

tbabej commented Jul 27, 2021

@smemsh I think you're correct here in your observations. Let's merge this patchset and perhaps we can open a separate issue for the work week discrepancies.

@tbabej tbabej merged commit 4f41223 into GothenburgBitFactory:master Jul 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
todo There is an action point associated with the issue/PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants