Skip to content

Wrong weekday range #90

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

Closed
agileshaw opened this issue Aug 13, 2024 · 7 comments
Closed

Wrong weekday range #90

agileshaw opened this issue Aug 13, 2024 · 7 comments

Comments

@agileshaw
Copy link

The weekday range should be 0-6 as specified in Cron. However, croniter is accepting 7 as a valid weekday value.

Current:

>>> from croniter import croniter
>>> croniter.is_valid("* * * * 7")
True
>>>

Expecting:

>>> from croniter import croniter
>>> croniter.is_valid("* * * * 7")
False
>>>

The line causing the error seems to be: https://github.com/kiorky/croniter/blob/4d6bb9747ddaaaf5cf44af7e4e821065cf9969fa/src/croniter/croniter.py#L140

kiorky added a commit that referenced this issue Oct 28, 2024
@kiorky kiorky closed this as completed in 3141708 Oct 28, 2024
@kiorky
Copy link
Collaborator

kiorky commented Oct 28, 2024

Well, 7 is an alias to day 0, see: https://github.com/kiorky/croniter/blob/3.0.4/src/croniter/croniter.py#L221
This is an old feature that i never was fond of, but the project is now dead.

I removed it in already released 4.0.0 to stick to standard cron.

@gibsondan
Copy link

Chiming in in support of the previous behavior after we got at least one bug report after upgrading - That same wiki page from the original post here indicates that 7 is allowed on some systems, and I think most Linux crontabs support it.

@WillDaSilva
Copy link

WillDaSilva commented Oct 28, 2024

I'll add that we can't change how our library handles cron expressions without making a major version bump ourselves. To avoid that, but still upgrade to croniter v4, we'll have to add code that replaces 7 in the day-of-week field with 0 works around 7 not being supported.

I haven't fully thought through whether having the 7 be an alias for 0 is a good idea in a greenfield project, but since this isn't a greenfield project, and instead has many users, the upgrade to croniter v4 will likely cause bugs in lots of software that uses it, or else impose a burden as everyone who uses it who doesn't want to break things has to implement workarounds to preserve the historical behaviour.

Regardless, @kiorky, you have my thanks for maintaining this useful project.

@gibsondan
Copy link

gibsondan commented Oct 28, 2024

(+1 re: thanks!)

I thought about swapping in 0 for 7 as well but wasn't immediately sure if that would break "6-7" or not, haven't had a chance to test to confirm yet. Maybe swapping in the day of the week strings (6 => Sat, 7 = Sun, etc.)?

@kiorky
Copy link
Collaborator

kiorky commented Oct 29, 2024

I'll add that we can't change how our library handles cron expressions without making a major version bump ourselves. To avoid that, but still upgrade to croniter v4, we'll have to add code that replaces 7 in the day-of-week field with 0 works around 7 not being supported.

On that specific point, please note that a major version bump has been done, 3->4.

@kiorky
Copy link
Collaborator

kiorky commented Oct 29, 2024

I thought about swapping in 0 for 7 as well but wasn't immediately sure if that would break "6-7" or not, haven't had a chance to test to confirm yet. Maybe swapping in the day of the week strings (6 => Sat, 7 = Sun, etc.)?

If you had took the time to read last changelog, and last changesets along that release, you would have been insured of what behavior to expect around the SUNDAY ranges which indeed need particular care.
You can refer to the https://github.com/kiorky/croniter/blob/4.0.0/src/croniter/tests/test_croniter.py#L1979 test_sunday_ranges_to & test_sunday_ranges_from tests to know what to expect.

As of today, Sat-Sun or 6-0 would work meaning saturday followed by the nearest sunday, but not 0-6 as this would mean each day of week.

kiorky added a commit that referenced this issue Oct 29, 2024
kiorky added a commit that referenced this issue Oct 29, 2024
kiorky added a commit that referenced this issue Oct 29, 2024
kiorky added a commit that referenced this issue Oct 29, 2024
kiorky added a commit that referenced this issue Oct 29, 2024
kiorky added a commit that referenced this issue Oct 29, 2024
kiorky added a commit that referenced this issue Oct 29, 2024
@kiorky
Copy link
Collaborator

kiorky commented Oct 29, 2024

So as asked, DOW7 behavior is restored, and now documented and will stay as-is.
Also, ranges calculation have again been reworked to support all backtracking expressions (sat-sun, wed-sun, apr-dec, apr-feb, etc).
Please see documentation and tests added in 5.0.1: 4.0.0...5.0.1

WillDaSilva added a commit to meltano/meltano that referenced this issue Oct 29, 2024
pallets-eco/croniter#90 (comment)

Now that the old behviour has been restored, we can continue using the latest version.
github-merge-queue bot pushed a commit to meltano/meltano that referenced this issue Oct 29, 2024
pallets-eco/croniter#90 (comment)

Now that the old behviour has been restored, we can continue using the latest version.
kiorky added a commit that referenced this issue Dec 16, 2024
kiorky added a commit that referenced this issue Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants