GH-36 Changes for cron syntax compat (dayOfWeek 0-6 and no-seconds syntax support) #41

Merged
merged 3 commits into from Aug 7, 2012

Conversation

Projects
None yet
4 participants
@danhbear

Here are the changes for cron syntax compatibility. The parsing scenarios are really a matter of preference. I'm actually not certain what Unix cron does.

  • If less than 5 digits, should CronTime just throw an error immediately? Previously the code assumed the first n digits matched the beginning of a correct cron time. Now it assumes the first n digits match the end of a correct cron time. This is to support the standard 5-digit.
  • I included CronTime.parseDefaults to keep other digits' parse defaults the same as before. Not sure what desired behavior is here.

Personally as a consumer I'd prefer it throw for <5 to avoid any confusion on the above.

@travisbot

This comment has been minimized.

Show comment Hide comment
@travisbot

travisbot Jul 24, 2012

This pull request fails (merged 7102fdb into 36eb441).

This pull request fails (merged 7102fdb into 36eb441).

@danhbear

This comment has been minimized.

Show comment Hide comment
@danhbear

danhbear Jul 24, 2012

@ncb000gt Not sure why this is failing. I ran the tests locally and they passed on 3 of the 4 jobs.

@ncb000gt Not sure why this is failing. I ran the tests locally and they passed on 3 of the 4 jobs.

tests/test-cron.js
+ }, null, true);
+ setTimeout(function() {
+ c.stop();
+ // Note that it could be called once if the test lands on a minute boundary

This comment has been minimized.

Show comment Hide comment
@ncb000gt

ncb000gt Jul 26, 2012

Collaborator

You can account for this by checking to see if the test is within 6 seconds of the boundary. If it is, just set a timeout to fire after 5 seconds...many of the tests in this suite are like that due to the time sensitive nature of the tests. :)

@ncb000gt

ncb000gt Jul 26, 2012

Collaborator

You can account for this by checking to see if the test is within 6 seconds of the boundary. If it is, just set a timeout to fire after 5 seconds...many of the tests in this suite are like that due to the time sensitive nature of the tests. :)

@ncb000gt

This comment has been minimized.

Show comment Hide comment
@ncb000gt

ncb000gt Jul 26, 2012

Collaborator

The build looks like it failed after it checked out the changes for the tests with v0.8.x. Very strange. I can probably get the tests to run again...just gotta look into it.

Collaborator

ncb000gt commented Jul 26, 2012

The build looks like it failed after it checked out the changes for the tests with v0.8.x. Very strange. I can probably get the tests to run again...just gotta look into it.

@ncb000gt

This comment has been minimized.

Show comment Hide comment
@ncb000gt

ncb000gt Jul 26, 2012

Collaborator

Bleh, it looks like i can only force it to refresh master for the time being. If you make the change to the test to support the minute boundary it should kick it off again.

Collaborator

ncb000gt commented Jul 26, 2012

Bleh, it looks like i can only force it to refresh master for the time being. If you make the change to the test to support the minute boundary it should kick it off again.

@ncb000gt

This comment has been minimized.

Show comment Hide comment
@ncb000gt

ncb000gt Jul 26, 2012

Collaborator

For linkage. This fixes GH-36.

Collaborator

ncb000gt commented Jul 26, 2012

For linkage. This fixes GH-36.

@travisbot

This comment has been minimized.

Show comment Hide comment
@travisbot

travisbot Jul 26, 2012

This pull request passes (merged 8205bc7 into 36eb441).

This pull request passes (merged 8205bc7 into 36eb441).

@danhbear

This comment has been minimized.

Show comment Hide comment
@danhbear

danhbear Jul 26, 2012

Alrighty, travis test glitch seems fixed. Let me know if 8205bc7 addresses your comment in the CR.

Alrighty, travis test glitch seems fixed. Let me know if 8205bc7 addresses your comment in the CR.

@ncb000gt

This comment has been minimized.

Show comment Hide comment
@ncb000gt

ncb000gt Jul 26, 2012

Collaborator

That looks perfect. I had to do this back when I moved to Travis due to the fact that I couldn't time the tests myself. :)

I'll merge this into a branch for a bit more review and testing. Thanks!

Collaborator

ncb000gt commented Jul 26, 2012

That looks perfect. I had to do this back when I moved to Travis due to the fact that I couldn't time the tests myself. :)

I'll merge this into a branch for a bit more review and testing. Thanks!

@danhbear

This comment has been minimized.

Show comment Hide comment
@danhbear

danhbear Aug 7, 2012

Any update on merging this in?

danhbear commented Aug 7, 2012

Any update on merging this in?

@ncb000gt ncb000gt merged commit 8205bc7 into kelektiv:master Aug 7, 2012

@ncb000gt

This comment has been minimized.

Show comment Hide comment
@ncb000gt

ncb000gt Aug 7, 2012

Collaborator

Herp derp. I completely blanked on this. Too busy. :)

Anyway, it is in master and the tests are all passing. I'll be updating the readme and version number to point out the breaking changes (DoW). Then I'll publish the changes out.

Thanks again.

Collaborator

ncb000gt commented Aug 7, 2012

Herp derp. I completely blanked on this. Too busy. :)

Anyway, it is in master and the tests are all passing. I'll be updating the readme and version number to point out the breaking changes (DoW). Then I'll publish the changes out.

Thanks again.

@danhbear

This comment has been minimized.

Show comment Hide comment
@danhbear

danhbear Aug 7, 2012

Awesome, thanks! Yeah would be good to update the version or let people know, not sure the proper node etiquette. Depending on their npm usage of node-cron is seems like they might get in trouble. For example, we have some services that do an npm install/update during each service deploy.

danhbear commented Aug 7, 2012

Awesome, thanks! Yeah would be good to update the version or let people know, not sure the proper node etiquette. Depending on their npm usage of node-cron is seems like they might get in trouble. For example, we have some services that do an npm install/update during each service deploy.

@ncb000gt

This comment has been minimized.

Show comment Hide comment
@ncb000gt

ncb000gt Aug 7, 2012

Collaborator

Right. But if people are versioning their production code correctly then the package.json should use explicit versions. I'm unaware of any other way besides in the repository to really inform users that there are breaking changes.

Collaborator

ncb000gt commented Aug 7, 2012

Right. But if people are versioning their production code correctly then the package.json should use explicit versions. I'm unaware of any other way besides in the repository to really inform users that there are breaking changes.

@danhbear

This comment has been minimized.

Show comment Hide comment
@danhbear

danhbear Aug 7, 2012

Makes sense. I should probably avoid things like package.json entries like ">=0.1.13" and ">=0.0".

danhbear commented Aug 7, 2012

Makes sense. I should probably avoid things like package.json entries like ">=0.1.13" and ">=0.0".

@ncb000gt

This comment has been minimized.

Show comment Hide comment
@ncb000gt

ncb000gt Aug 7, 2012

Collaborator

I think you'll find that it will usually be fine, but there will be that one case where something changes in a very unexpected way and poof, no more DB or something like that. :) Only takes one of those to happen before you start getting REAL specific- not that I'm speaking from experience or anything...I mean, who loses data...really...cough

Collaborator

ncb000gt commented Aug 7, 2012

I think you'll find that it will usually be fine, but there will be that one case where something changes in a very unexpected way and poof, no more DB or something like that. :) Only takes one of those to happen before you start getting REAL specific- not that I'm speaking from experience or anything...I mean, who loses data...really...cough

@ncb000gt

This comment has been minimized.

Show comment Hide comment
@ncb000gt

ncb000gt Aug 7, 2012

Collaborator

I published this to NPM. Thank you for the code and enjoy! :)

Collaborator

ncb000gt commented Aug 7, 2012

I published this to NPM. Thank you for the code and enjoy! :)

@scien

This comment has been minimized.

Show comment Hide comment
@scien

scien Aug 14, 2012

Here's a great article for package dependencies standards. http://blog.nodejitsu.com/package-dependencies-done-right.

So I use entries like 0.2.x (major.minor.patch), with the (hopefully true) assumption that patches are non-breaking and good to have, but upgrade a major or minor version should be investigated/tested before integration.

scien commented Aug 14, 2012

Here's a great article for package dependencies standards. http://blog.nodejitsu.com/package-dependencies-done-right.

So I use entries like 0.2.x (major.minor.patch), with the (hopefully true) assumption that patches are non-breaking and good to have, but upgrade a major or minor version should be investigated/tested before integration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment