HangfireIO / Cronos

A fully-featured .NET library for working with Cron expressions. Built with time zones in mind and intuitively handles daylight saving time transitions
MIT License
1.01k stars 120 forks source link

CronExpression.ToString is not parseable anymore #15

Closed Horusiath closed 10 months ago

Horusiath commented 5 years ago

It looks like a ToString method is not symetric - what I mean by that is that one would expect from CronExpression to pass through Parse→ToString→Parse cycle to produce an equivalent expression as in the first parse.

The motivation for that is that stringified cron expression may be i.e. serialized and restored later.

Simple example to reproduce an issue:

var expr = CronExpression.Parse("*/5 * * * *");
var serialized = expr.ToString(); // 0 0,5,10,15,20,25,30,35,40,45,50,55 * * * *
var expr2 = CronExpression.Parse(serialized);
/*
Cronos.CronFormatException
Days of month: Value must be a number between 1 and 31 (all inclusive).
   at Cronos.CronExpression.ThrowFormatException(CronField field, String format, Object[] args)
   at Cronos.CronExpression.ParseDayOfMonth(Char*& pointer, CronExpressionFlag& flags, Byte& lastDayOffset)
   at Cronos.CronExpression.Parse(String expression, CronFormat format)
*/

I think, the easiest way to solve this, would be to attach original cron expression string as an extra field of the CronExpression class:

jbennink commented 1 year ago

It looks like ToString() returns a cronexpression with seconds, hence the extra 0 in front. It looks like internally the fact that seconds were not included in the cronexpression parsed is not stored hence upon returning the ToString() it is not possible to differentiate between no seconds supplied and 0 seconds, which results in the 0 in the result.

P.S. The current implementation is not wrong, it might however be unexpected. P.P.S. It is a different matter whether you want the exact same syntax back */5 instead of 0,5,10,15,20,25,30,35,40,45,50,55 which is again a decision, both express the same cronexpression. P.P.P.S. Optimizing for ToString() speed is again, another matter and a separate thing from what should be returned from ToString() Since the implementation already does some bit-shift magic and explicitly mention storage bit sizes I guess someone might object to any bytes of additional memory consumed for optimizing ToString()

@odinserj If replies like this are not appreciated please say so. I was not trying to give opinionated answers or comments