bradymholt / cRonstrue

JavaScript library that translates Cron expressions into human readable descriptions
http://bradymholt.github.io/cRonstrue/
MIT License
1.25k stars 166 forks source link

Daily cron expression with timezone offset displays incorrect day #313

Open khOst opened 6 months ago

khOst commented 6 months ago

Cron Expression 0 20 28 * * with a tzOffset set to 5

Expected Output "At 01:00 AM, on day 29 of the month"

Actual Output "At 01:00 AM, on day 28 of the month"

Prerequisites

bradymholt commented 6 months ago

Isn't this correct? With tzOffset set to 5 and hour set to 20, this would cause the time to move to the next day. This was implemented / changed in https://github.com/bradymholt/cRonstrue/pull/296.

khOst commented 6 months ago

Sorry, expected and actual should be switched in places, I've updated the description. The time is expected to move to next day, but cRonstrue returns the same day, despite the offset.

khOst commented 6 months ago

Also for a negative tzOffset instead of returning the same day, it should return the previous day. For example, day 27 is expected here https://runkit.com/65a61ad4cca25b00082679bc/65a61ae3fdc4a20008bebfa5.

bradymholt commented 6 months ago

@maxtwardowski - Do you think changes in https://github.com/bradymholt/cRonstrue/pull/296 need to be tweaked for this case?

ZaneGeiser commented 3 months ago

Hi @bradymholt, I just started using this package (tyvm for building it!) this week and noticed this same issue.

I wrote some test that I think would need to pass in order for this issue to be resolved.

    it("5 23 1 * *", function() {
      assert.equal(cronstrue.toString(this.test?.title as string, {
        tzOffset: 2,
      }), "At 01:05 AM, on day 2 of the month")
    });

    it("5 23 31 * *", function() {
      assert.equal(cronstrue.toString(this.test?.title as string, {
        tzOffset: 2,
      }), "At 01:05 AM, on day 1 of the month")
    });

    it("5 23 1 * *", function() {
      assert.equal(cronstrue.toString(this.test?.title as string, {
        tzOffset: -2,
      }), "At 11:05 PM, on day 31 of the month")
    });

I went to try making the code change but noticed there are some special day-of-month cases (like L, WL, and LW), and wasn't sure how it would need to fit in with those cases.

ZaneGeiser commented 3 months ago

As I think about it more, it probably needs to wrap the L day of the month forward to 1 if the tzOffset pushes over the dateline.

Ie, if L is converted to the day before the last day, then it should remain as the Last day. But if L is converted to the day after the last day then it should write 'day 1 of the month'.

Or as tests:

    it("5 23 L * *", function() {
      assert.equal(cronstrue.toString(this.test?.title as string, {
        tzOffset: -2,
      }), "At 01:05 AM, on the last day of the month")
    });

    it("5 23 L * *", function() {
      assert.equal(cronstrue.toString(this.test?.title as string, {
        tzOffset: 2,
      }), "At 01:05 AM, on day 1 of the month")
    });
kbazzani commented 4 weeks ago

First, very useful package, keep up the good work!

Second, here are a couple more examples. Along with the day of the month as previously mentioned, the month also doesn't wrap correctly.

construe.toString( "59 23 31 12 3", { locale:"en", tzOffset: 1} ) 'At 12:59 AM, on day 31 of the month, and on Thursday, only in December'

Should be Jan 1st

construe.toString( "01 00 01 01 3", { locale:"en", tzOffset: -1} ) 'At 11:01 PM, on day 01 of the month, and on Tuesday, only in January'

Should be Dec 31st.

Best!