 |
History
| Log In
|
|
|
|
I agree with Aaron's comments. It does not look like a trivial change to support this, and I'd like to wrap 1.6.1 up soon.
Suggest we go with throwing an exception stating that the expression is not supported, and clarify the javadoc relating to "L". +1 on an exception. ParseException, or UnsupportedOperationException?
Seems like we've a bunch of options to check to see if they work: L in the DAY_OF_MONTH (content of this issue) L in the DAY_OF_WEEK (title of this issue) W in the DAY_OF_MONTH =-= If we rewrite CronExpression; we need to do it such that it's very, very easy to unit test. The time nature of Quartz makes unit testing quite hard, so I suspect you have to bake unit testing in very strongly to the design. I'd go with ParseException, and update the docs for those two characters to be clear (if not already) what fields they are valid for, and the limitation of not being part of a list. -- Yes, unit testing IS very hard, and wasn't so fashionable in 2001 - though at the time I did give it a lot of thought (which is where the old TimeBroker interface came from - thinking it would be nice to be able to simulate time movement) -- but I pretty much gave up due to the headaches (conceptual and otherwise) involved with it all. With a re-do, accommodating testing will definitely have to be built into the design. james Attaching patch to throw exceptions when L is with other entries in the day-of-month or day-of-week fields. The LW case is taken care of automatically by doing that.
New issue created to implement this feature: QUARTZ-663
Exception added from patch: svn ci -m "Adding an exception throwing when the day of week or day of month contains both an L and numbers as that is not (yet) supported. Reported in Sending src/java/org/quartz/CronExpression.java Sending src/test/java/org/quartz/CronExpressionTest.java Transmitting file data .. Committed revision 808. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Bug
Closed
Major
The Cron Expression do not support day of week list with "L" in it.
protected Date getTimeAfter(Date afterTime)
method. Ostensibly, this could be fixed, but I think it would require a significant rewrite of a section of the code in that method. I would suggest we take the second option (throw an exception) as a temporary measure and then create a new entry slated for a later release -- to rewrite CronExpression...
I don't say this because I think it could necessarily be implemented any more correctly (I'm not sure whether it can or not). Rather, I find the class nearly unmanageable because the code is difficult to read. The use of extremely abbreviated variable names makes it difficult to maintain. We also may be able to fully implement some of the features that have not been available too (i.e. simultaneous use of the day-of-month and day-of-week fields). Though I have to say that support for the 'C' character should probably never be supported.
The downside would, of course, be that a newly rewritten CronExpression would need to be extensively tested, as it forms the core of one of Quartz's most popular (if misunderstood) features.
In any case, even if we didn't rewrite CronExpression, the Exception could act as a good temporary 'bandaid' until the getTimeAfter() method could be corrected.