Primarily Calculate cron time vs setInterval

[crcn]

No description provided.

[ncb000gt]

This looks great.

However, I don't see any additional tests nor do I see rewrites of existing tests to support these changes. What I'll likely do is use this as a catalyst for the next version of the cron lib. Since there are other features I'd like to add. However, at the very least, I wont release this until the tests are reworked.

Additionally, because this breaks API (the completed 3rd param), it should have a major version bump. I haven't exactly been good about this in the past. =\

Also, The original copyright is to James Padolsey as he wrote the original bit with myself and a couple others adding some features and maintaining it. As such, I'll be changing the copyright notice.

Thanks for the work! I'll close it when things are finished up and have taken shape.

[crcn]

Whoops - apologies for adding the copyright in there - it shouldn't be there.

Also, I just figured out pull requests. The list of changes wouldn't have been so big if I knew what it was earlier.

I'll add my test-code next time I submit changes. Thanks for the feedback!

[ncb000gt]

All good. I don't mind large change sets. Some people do and would ask you to squash them into one commit, but meh. :) I prefer to see the whole history.

I did push to a branch called GH-10 to link it with this pull request. This changeset also appears to take care of GH-9. Woot.

[crcn]

Perfect. I'll get around to adding some tests sometime within the next week
or so.

-Craig

On Fri, Sep 23, 2011 at 11:43 AM, Nick Campbell <
reply@reply.github.com>wrote:

All good. I don't mind large change sets. Some people do and would ask you
to squash them into one commit, but meh. :) I prefer to see the whole
history.

I did push to a branch called GH-10 to link it with this pull request. This
changeset also appears to take care of GH-9. Woot.

Reply to this email directly or view it on GitHub:
https://github.com/ncb000gt/node-cron/pull/10#issuecomment-2180475

[ncb000gt]

I added a few more commits to that branch to fix up testing and to change the code to support those tests. Also, sendAt used to take a param but that was never used. I changed that instead to use this.source as I expect that was the intention.

Along with this change I'm setting the code up to allow for more changes. Namely, CronTime will also be able to take a Date object to support non-cron syntax. This was requested at https://github.com/hookio/cron/issues/4 by @Marak and I think allowing a Date object provides a good medium between cron style and normal date time styling. Additionally someone could use Datejs which allows for syntax like "next wednesday" and so on in the date formatting.

This has been merged into master. Still needs work and more testing but the existing tests should pass when run now.

[crcn]

Looks great.

Regarding the method "sendAt" - the name might be a bit confusing. It's actually used to calculate the time from either now, or the "start" date. I'll make a few changes so the impl. is more clear.

[ncb000gt]

Excellent! Thanks.