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: fix range parsing when upper limit = 0 #687

Merged
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

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
10 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
## [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 added a commit to sheerlox/node-cron that referenced this pull request Sep 10, 2023
ncb000gt pushed a commit that referenced this pull request Sep 10, 2023
## [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
## [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 pushed a commit that referenced this pull request Sep 27, 2023
## [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 to sheerlox/node-cron that referenced this pull request Sep 27, 2023
sheerlox pushed a commit to sheerlox/node-cron that referenced this pull request Sep 27, 2023
## [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 pushed a commit to sheerlox/node-cron that referenced this pull request Sep 29, 2023
## [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 sheerlox removed their assignment Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released type:bug Bug reports and bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] 2-0 week expression error
3 participants