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.

Sign up for GitHub

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

Jump to bottom

fix: fix range parsing when upper limit = 0 #687

Merged
merged 3 commits into from
Aug 26, 2023
Merged

fix: fix range parsing when upper limit = 0 #687

merged 3 commits into from
Aug 26, 2023

Conversation

sheerlox
Copy link
Collaborator

Description

When providing the invalid value 0 as the upper limit in a range (e.g. 2-0), the library was not detecting the error due to the use of the || operator, which considers 0 & undefined the same way.
I have replaced the operator to check specifically for undefined in order to detect the invalid value 0.

Related Issue

Fixes #654.

How Has This Been Tested?

I have extended an existing test case to check for invalid ranges and added a test case that checks all valid ranges are accepted.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • If my change introduces a breaking change, I have added a ! after the type/scope in the title (see the Conventional Commits standard).

@sheerlox sheerlox self-assigned this Aug 15, 2023
@sheerlox sheerlox added the type:bug Bug reports and bug fixes label Aug 15, 2023
@sheerlox
Copy link
Collaborator Author

I'm not sure whether to consider this as a breaking change or not 🤔

On one hand, it will break the code of anyone using ranges like 3-0, but on the other hand it's an invalid cron expression and the behavior when using such a range is unknown and doesn't achieve the "expected" result (see #654).

So I'd be tempted to say we release this as a fix and not as a breaking change, wdyt?

@intcreator
Copy link
Collaborator

I think because it's not documented to work the way that it does it shouldn't be considered a breaking change. it was never officially acknowledged as being part of the API. so my vote is bug fix over breaking change. you shouldn't depend on undefined behavior in your code

intcreator
intcreator approved these changes Aug 26, 2023
View reviewed changes
Copy link
Collaborator

@intcreator intcreator left a comment

Choose a reason for hiding this comment

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

I know it's fixing a bug but I think being explicit helps the code be more readable. it's possible we might could use ?? here instead of || and && but since we want a boolean anyway I think your change is best

tests look good too!

@sheerlox sheerlox merged commit d96746f into kelektiv:main Aug 26, 2023
11 checks passed
@sheerlox sheerlox deleted the fix/654/fix-range-parsing branch August 26, 2023 16:42
ncb000gt pushed a commit that referenced this pull request Aug 26, 2023
@semantic-release-bot
Release v2.4.3 [skip ci]
0317684 
## [2.4.3](v2.4.2...v2.4.3) (2023-08-26)

### 🐛 Bug Fixes

* fix range parsing when upper limit = 0 ([#687](#687)) ([d96746f](d96746f))

### 🚨 Tests

* add TS types check ([#690](#690)) ([f046016](f046016))
@ncb000gt
Copy link
Member

🎉 This PR is included in version 2.4.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

@sheerlox sheerlox mentioned this pull request Sep 9, 2023
Open
34 tasks
sheerlox added a commit to sheerlox/node-cron that referenced this pull request Sep 10, 2023
@sheerlox
fix: fix range parsing when upper limit = 0 (kelektiv#687)
14d7fa8
ncb000gt pushed a commit that referenced this pull request Sep 10, 2023
@semantic-release-bot
Release v3.0.0-beta.4 [skip ci]
ae16565 
## [3.0.0-beta.4](v3.0.0-beta.3...v3.0.0-beta.4) (2023-09-10)

### 🐛 Bug Fixes

* **deps:** update dependency luxon to v3.3.0 & add [@types](https://github.com/types)/luxon ([#689](#689)) ([c95a449](c95a449)), closes [#688](#688)
* fix range parsing when upper limit = 0 ([#687](#687)) ([d96746f](d96746f))

### 🚨 Tests

* add TS types check ([#690](#690)) ([f046016](f046016))
@ncb000gt
Copy link
Member

🎉 This PR is included in version 3.0.0-beta.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

patrickm68 added a commit to patrickm68/node-cron-types that referenced this pull request Sep 14, 2023
@patrickm68
Release v2.4.3 [skip ci]
eb90872 
## [2.4.3](kelektiv/node-cron@v2.4.2...v2.4.3) (2023-08-26)

### 🐛 Bug Fixes

* fix range parsing when upper limit = 0 ([#687](kelektiv/node-cron#687)) ([d96746f](kelektiv/node-cron@d96746f))

### 🚨 Tests

* add TS types check ([#690](kelektiv/node-cron#690)) ([f046016](kelektiv/node-cron@f046016))
sheerlox added a commit that referenced this pull request Sep 27, 2023
@sheerlox
fix: fix range parsing when upper limit = 0 (#687)
c2ca3dd
sheerlox pushed a commit that referenced this pull request Sep 27, 2023
@semantic-release-bot @sheerlox
Release v2.4.3 [skip ci]
22f3ec4 
## [2.4.3](v2.4.2...v2.4.3) (2023-08-26)

### 🐛 Bug Fixes

* fix range parsing when upper limit = 0 ([#687](#687)) ([d96746f](d96746f))

### 🚨 Tests

* add TS types check ([#690](#690)) ([f046016](f046016))
sheerlox added a commit that referenced this pull request Sep 27, 2023
@sheerlox
fix: fix range parsing when upper limit = 0 (#687)
1407f1f
sheerlox added a commit to sheerlox/node-cron that referenced this pull request Sep 27, 2023
@sheerlox
fix: fix range parsing when upper limit = 0 (kelektiv#687)
510fb0f
sheerlox pushed a commit to sheerlox/node-cron that referenced this pull request Sep 27, 2023
@semantic-release-bot @sheerlox
Release v2.4.3 [skip ci]
8f26b7c 
## [2.4.3](kelektiv/node-cron@v2.4.2...v2.4.3) (2023-08-26)

### 🐛 Bug Fixes

* fix range parsing when upper limit = 0 ([kelektiv#687](kelektiv#687)) ([d96746f](kelektiv@d96746f))

### 🚨 Tests

* add TS types check ([kelektiv#690](kelektiv#690)) ([f046016](kelektiv@f046016))
sheerlox added a commit to sheerlox/node-cron that referenced this pull request Sep 29, 2023
@sheerlox
fix: fix range parsing when upper limit = 0 (kelektiv#687)
b016dde
sheerlox pushed a commit to sheerlox/node-cron that referenced this pull request Sep 29, 2023
@semantic-release-bot @sheerlox
Release v2.4.3 [skip ci]
b69affd 
## [2.4.3](kelektiv/node-cron@v2.4.2...v2.4.3) (2023-08-26)

### 🐛 Bug Fixes

* fix range parsing when upper limit = 0 ([kelektiv#687](kelektiv#687)) ([d96746f](kelektiv@d96746f))

### 🚨 Tests

* add TS types check ([kelektiv#690](kelektiv#690)) ([f046016](kelektiv@f046016))
sheerlox added a commit to sheerlox/node-cron that referenced this pull request Sep 29, 2023
@sheerlox
fix: fix range parsing when upper limit = 0 (kelektiv#687)
89dc2f4
@sheerlox sheerlox removed their assignment Oct 23, 2023
@j-mendez j-mendez mentioned this pull request May 25, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@intcreator intcreator intcreator approved these changes

Assignees
No one assigned
Labels
released type:bug Bug reports and bug fixes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] 2-0 week expression error
3 participants
@sheerlox @intcreator @ncb000gt