harrisiirak / cron-parser

Node.js library for parsing crontab instructions
MIT License
1.3k stars 155 forks source link

dayOfWeek: [7] is not parsed with fieldsToExpression().stringify() #352

Open arwa9 opened 1 month ago

arwa9 commented 1 month ago

When adding Sunday as value 7 inside daysOfWeek of the CronFields, and later parsing it to a cron expression with fieldsToExpression().stringify(), it is ignored.

Following the "Manipulation" example in the documentation:

var interval = parser.parseExpression('0 7 * * 0-4');
var fields = JSON.parse(JSON.stringify(interval.fields)); // Fields is immutable
fields.hour = [8];
fields.minute = [29];
fields.dayOfWeek = [1,3,4,5,6,7];
var modifiedInterval = parser.fieldsToExpression(fields);
var cronString = modifiedInterval.stringify();
console.log(cronString); // "29 8 * * 1,3-7"

When running said code, the actual output of that console.log() is "29 8 * * 1,3-6".

Seeing this should be handled here but doesn't appear to be working: https://github.com/harrisiirak/cron-parser/blob/master/lib/expression.js#L343

Also possible missing definition of sunday as 7 here (?): https://github.com/harrisiirak/cron-parser/blob/master/lib/expression.js#L109

harrisiirak commented 1 month ago

Hi @arwa9!

When parsing an expression, as fallback it supports day of week as range from 0-7. However, by the spec, day of week is defined as SUN - SAT, which translates to numeric range 0-6 (where 0 is SUN).

When serializing/stringifying interval, there is explicit behaviour to ignore 7 as day of week value.

If you want retain Sunday as part of serialized expression, Sunday must be presented as 0 and not 7.

Best regards

arwa9 commented 1 month ago

Thank you for your quick response.

That's what I ended up doing, but then using a cron expression "translator" to human-readable expressions (like cronstrue ), this had the implication that those sentences become like "sunday and thursday to saturday" instead of "thursday to sunday".

Seeing the linux crontab reference I see the expected indexes for weekdays are indeed 0 to 6, but I became confused by the same exact code of your documentation I pasted in the issue, which I ran in an empty project and the output is not the expected as I mentioned.

If then using Sunday as 7 inside dayOfWeek for parsing purposes is purposefully being ignored, maybe it would be nice to not have that as an example in the documentation since it might lead to misunderstandings such as mine.

Thank you again for your time and keep up the good work!