Strict Matching #29
Strict Matching #29
Conversation
ensures that other cron entries are unimpacted by the flag, if they should not be
|
Hey Russell, First of all: thanks for looking into that issue! I aggree with your thinking that one should be able to express friday the 13th, but the spec itself doesn't allow this (as far as I can see). Your idea of switching the "new" behaviour (why did you call it I'll add general feedback to the code directly in the code. |
|
|
||
| def set_strict_match(strict_match = false) | ||
| @strict_match = false | ||
| if strict_match && [time_specs[:dom].last, time_specs[:dow].last].all?{|v| v != '*'} |
There was a problem hiding this comment.
Is that check even needed? I mean if the user tells us to use strict matching, why would we refuse that? I'd rather do:
attr_accessor :strict_match
alias_method :strict_match?, :strict_matchThis would even get rid of the predicate method
There was a problem hiding this comment.
Actually yes; when I did not have this check in place, other specs failed that should not have. I'll admit I only 60-70% get whats actually going on with the code, so maybe you'd know better than I why. or, maybe it was from something else I later undid. I'll see what happens when i remove it again.
There was a problem hiding this comment.
Yeah I just notifed when reading through the whole file that this beast is in serious need of a refactoring.
My plan yould be to chunk it up into understandable pieces, like having a CronParser::Default and CronParser::Strict class. But for that, lots of code would need to be changed, so I would do that after incorporating your changes.
|
@siebertm thanks for reviewing this! much appreciated. I called it "strict" because it's a strict match. But you're right, it's made up. this is outside of the cron spec, but behavior that I wish it could do, instead of what it does now which I doubt anyone has ever actually used, because its a weird match condition to rely on. |
|
@siebertm i updated the PR to remove the inverted if blocks. As for the removal of the check, and using attr_accessor, I was unable to get that to work sanely. It really does need to only be strict when both are wildcard, any other condition, with it enabled, can result in weird behavior. Sorry for the long turn around! Let me know if there's anything else I need to update. |
This PR adds strict matching to cron parsing.
I find the default matching behavior when both DOW and DOM are specified to be unexpected. It inhibits a number of use cases, like matching Friday the 13th, or the second tuesday in a month (when i have philly.rb meetings!). So, I added an optional flag to support "strict matching" where both DOM and DOW must match.
Added spec coverage for behavior, and updated Readme to reflect. Happy to discuss any changes or make additional ones to get this merged in.