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: added fractional offset support #685

Merged
merged 14 commits into from
Sep 25, 2023

Conversation

rharshit82
Copy link
Contributor

@rharshit82 rharshit82 commented Aug 12, 2023

Description

This is a fix for Issue - #568
There was a stale pull request in the past but since it was not active, made a fresh PR with support for negative as well as positive offset with minute support - #685

Related Issue

Fixes #568.

Motivation and Context

It allows negative as well as positive offset with minute spport

How Has This Been Tested?

Tested in node 16.16.0, ran all the jet test cases and added 2 test cases where it was needed

Screenshots (if appropriate):

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).

@rharshit82 rharshit82 changed the title added fractional offset support fix - added fractional offset support Aug 12, 2023
@rharshit82 rharshit82 changed the title fix - added fractional offset support fix : added fractional offset support Aug 12, 2023
@rharshit82 rharshit82 changed the title fix : added fractional offset support fix: added fractional offset support Aug 12, 2023
bug fixes

modified test cases for minute support
lib/time.js Outdated Show resolved Hide resolved
lib/time.js Outdated Show resolved Hide resolved
@sheerlox sheerlox changed the title fix: added fractional offset support fix!: added fractional offset support Aug 13, 2023
@sheerlox
Copy link
Collaborator

I've updated the PR title since this PR will introduce a breaking change: previously strings were parsed as minutes, now they would be parsed as "HH:mm" strings.

lib/time.js Show resolved Hide resolved
@rharshit82
Copy link
Contributor Author

Also, wanted to highlight here that previously also string behaviour worked fine, just minute support was not there.
Offset : "5:30" translates to "UTC+5"
Offset: "330" also translates to "UTC+5"

tests/cron.test.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@sheerlox sheerlox left a comment

Choose a reason for hiding this comment

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

left a few comments requesting some changes, otherwise lgtm 👍

@sheerlox
Copy link
Collaborator

sheerlox commented Aug 13, 2023

Also, wanted to highlight here that previously also string behaviour worked fine, just minute support was not there. Offset : "5:30" translates to "UTC+5" Offset: "330" also translates to "UTC+5"

oh my .. you're absolutely right, "magic" code right here ..

the kind of hidden behaviors that I was talking about earlier, those can really be a real pain 🤦

especially since the doc clearly states:

utcOffset - [OPTIONAL] - This allows you to specify the offset of your time zone rather than using the timeZone param. This should be an integer amount representing the number of minutes offset (like 120 for +2 hours or -90 for -1.5 hours) Probably don't use both timeZone and utcOffset together or weird things may happen."

so one might be tempted to use "30" for 30 minutes, then get an offset of 30 hours 🤣

@rharshit82
Copy link
Contributor Author

Have done the changes and tested it. Let me know if documentation changes are required for string support.

One thing to note is if we are adding string support, there will be cases where someone might put '330' as string offset where our code will take 330 hours as offset. I am thinking of handling this by using string as minutes in cases where : is not provided. What do you think?

@sheerlox
Copy link
Collaborator

Have done the changes and tested it. Let me know if documentation changes are required for string support.

Thanks for your changes, and yes, please update the documentation to reflect your changes.

One thing to note is if we are adding string support, there will be cases where someone might put '330' as string offset where our code will take 330 hours as offset. I am thinking of handling this by using string as minutes in cases where : is not provided. What do you think?

I think the best for now is to avoid introducing a breaking change in this PR, so we can release it on the v2 while we work on v3. I'll convert the codebase to Typescript soon, and I will be handling that issue at that time, which will then be released on v3 :)

@sheerlox sheerlox changed the title fix!: added fractional offset support fix: added fractional offset support Aug 13, 2023
@sheerlox
Copy link
Collaborator

Well actually there's a breaking change since you removed the "magic" code I referenced earlier (treating utcOffset as hours if it's <-60 || >60 ..

What do you think about reintroducing that behaviour for now, so this PR contains no breaking change, then start working on another PR targeting the beta branch to start redesigning that feature?

@sheerlox
Copy link
Collaborator

I ran the test pipeline for this PR, and there's some codestyle compliance issues.

You can run npm run lint -- --fix locally to autofix some of the issues, but there's (at least) one that'll still need to be fixed manually (your console will tell you which one after running the command).

@rharshit82
Copy link
Contributor Author

Thank you for reviewing the PR in such a short time. I will be doing all the changes asked above.

@intcreator
Copy link
Collaborator

I took a look as well. good conversation and this PR is moving in a good direction. I just made one small suggestion

lib/time.js Outdated Show resolved Hide resolved
@rharshit82 rharshit82 marked this pull request as draft August 15, 2023 15:15
@sheerlox sheerlox added the type:bug Bug reports and bug fixes label Aug 15, 2023
@rharshit82 rharshit82 marked this pull request as ready for review September 8, 2023 05:43
@rharshit82
Copy link
Contributor Author

We should definitely work on changing the dependence on local time zones for test cases in the future as it is very easy to miss edge cases. I checked your changes, Since only HH is supported in the string utcOffset, the test case does not support +xx:yy where yy is >0 and < 60. My timezone is 5:30 so the test case is not passing on my machine.
Have solved this in this Pr - https://github.com/rharshit82/node-cron/pull/1/files. Please review this and approve it so that I can merge it otherwise you can also make this change in this PR.

@sheerlox
Copy link
Collaborator

good catch, I didn't put much effort into that test since it's going to be removed soon. I've reviewed and tested your changes, I'll merge your fork PR now.

@sheerlox sheerlox mentioned this pull request Sep 25, 2023
9 tasks
@sheerlox sheerlox merged commit ce78478 into kelektiv:main Sep 25, 2023
10 checks passed
ncb000gt pushed a commit that referenced this pull request Sep 25, 2023
## [2.4.4](v2.4.3...v2.4.4) (2023-09-25)

### 🐛 Bug Fixes

* added fractional offset support ([#685](#685)) ([ce78478](ce78478))
@ncb000gt
Copy link
Member

🎉 This PR is included in version 2.4.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

ncb000gt pushed a commit that referenced this pull request Sep 25, 2023
## [3.0.0-beta.5](v3.0.0-beta.4...v3.0.0-beta.5) (2023-09-25)

### 🐛 Bug Fixes

* added fractional offset support ([#685](#685)) ([ce78478](ce78478))
@ncb000gt
Copy link
Member

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

The release is available on:

Your semantic-release bot 📦🚀

sheerlox added a commit to sheerlox/node-cron that referenced this pull request Sep 25, 2023
sheerlox added a commit to sheerlox/node-cron that referenced this pull request Sep 25, 2023
sheerlox added a commit to sheerlox/node-cron that referenced this pull request Sep 25, 2023
sheerlox added a commit to sheerlox/node-cron that referenced this pull request Sep 25, 2023
@sheerlox sheerlox mentioned this pull request Sep 27, 2023
9 tasks
sheerlox pushed a commit that referenced this pull request Sep 27, 2023
sheerlox pushed a commit that referenced this pull request Sep 27, 2023
## [2.4.4](v2.4.3...v2.4.4) (2023-09-25)

### 🐛 Bug Fixes

* added fractional offset support ([#685](#685)) ([ce78478](ce78478))
sheerlox pushed 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.4](kelektiv/node-cron@v2.4.3...v2.4.4) (2023-09-25)

### 🐛 Bug Fixes

* added fractional offset support ([kelektiv#685](kelektiv#685)) ([ce78478](kelektiv@ce78478))
sheerlox pushed 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.4](kelektiv/node-cron@v2.4.3...v2.4.4) (2023-09-25)

### 🐛 Bug Fixes

* added fractional offset support ([kelektiv#685](kelektiv#685)) ([ce78478](kelektiv@ce78478))
sheerlox pushed a commit to sheerlox/node-cron that referenced this pull request Sep 29, 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.

unable to use utcOffset
4 participants