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

Fix bump segments iteration order and day of month/week intersection/union handling #35

Merged
merged 9 commits into from
Mar 12, 2024

Conversation

codenem
Copy link
Contributor

@codenem codenem commented Mar 5, 2024

Fixes #33

Issue is that sometimes changing a part of the ref date should also change other parts.
Example: If we bump year while computing next tick, we must then put ref to 1st January of next year, at midnight.
To implement this logic the iteration over segments have been reversed to start with segments corresponding to bigger time periods, and the bump function has been updated to handle this new logic of dependent segment changes.

Fixes #26

Issue was that when using steps for months (e.g. 3/*) the fact that months start at 1 (January) and not 0 must be taken into account.

About day of week and day of month

So according to crontab guru (see tip 1 in https://crontab.guru/tips.html) and also general crontab standard, day of week and day of month are intertwined. So if any starts with * the computed day must match the intersection of the two. If not, it must match the union.

@codenem codenem requested a review from adhocore as a code owner March 5, 2024 09:27
@adhocore
Copy link
Owner

adhocore commented Mar 5, 2024

wow, thanks for this 👍. will take a look closely later when im in a PC

@codenem codenem force-pushed the fix-bump-segments-iteration-order branch from 979e74f to 71a9be7 Compare March 5, 2024 10:37
@codecov-commenter
Copy link

codecov-commenter commented Mar 6, 2024

Codecov Report

Attention: Patch coverage is 96.93878% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 93.88%. Comparing base (d3b6afc) to head (d971949).
Report is 2 commits behind head on main.

Files Patch % Lines
next.go 95.23% 1 Missing and 2 partials ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #35      +/-   ##
==========================================
- Coverage   94.78%   93.88%   -0.90%     
==========================================
  Files           9        9              
  Lines         594      671      +77     
==========================================
+ Hits          563      630      +67     
- Misses         16       22       +6     
- Partials       15       19       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@adhocore
Copy link
Owner

adhocore commented Mar 6, 2024

i have yet to review but wanted to let the actions run thru. it's great that tests/builds passed, only a little comment from coverage bot codecov.

@codenem
Copy link
Contributor Author

codenem commented Mar 6, 2024

i have yet to review but wanted to let the actions run thru. it's great that tests/builds passed, only a little comment from coverage bot codecov.

Understood thanks! I will add the missing coverage.

@codenem
Copy link
Contributor Author

codenem commented Mar 7, 2024

@adhocore added new tests for coverage

@@ -304,8 +304,8 @@ Following modifiers supported
- `L` stands for last day of month (eg: `L` could mean 29th for February in leap year)
- `W` stands for closest week day (eg: `10W` is closest week days (MON-FRI) to 10th date)
- *Day of Week / 5th of 5 segments / 6th of 6+ segments:*
- `L` stands for last weekday of month (eg: `2L` is last monday)
- `#` stands for nth day of week in the month (eg: `1#2` is second sunday)
- `L` stands for last weekday of month (eg: `2L` is last tuesday)
Copy link
Owner

@adhocore adhocore Mar 10, 2024

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@codenem codenem Mar 11, 2024

Choose a reason for hiding this comment

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

Yes both exists depending what tool you use, e.g. linux crontab uses 0 or 7 for Sunday (man 5 crontab).
Since the gronx code has been using 0 or 7 for Sunday (because it checks with go's time Weekday() method), I fixed the readme accordingly.

Copy link
Owner

Choose a reason for hiding this comment

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

alright then (im also thinking for a feature in yet another release to make configurable for whether to use 0 based or 1 based for ordinal modifiers - but it could be overkill given the gronx is intended to be a lean/minimal lib)

gronx.go Outdated Show resolved Hide resolved
gronx.go Show resolved Hide resolved
@adhocore adhocore changed the title Fix bump segments iteration order Fix bump segments iteration order and day of month/week intersection/union handling Mar 10, 2024
next.go Outdated Show resolved Hide resolved
@adhocore
Copy link
Owner

thank you for clarification. seems now we are near merging this PR. while it is more of personal preference, is there a way to make some var name shorter instead like monthDaySegIsIntersecting

@adhocore adhocore merged commit f720d7e into adhocore:main Mar 12, 2024
10 checks passed
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.

Inconsistencies with crontab.guru The day of Month goes wrong with L last day of month
3 participants