-
-
Notifications
You must be signed in to change notification settings - Fork 395
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Factorize common DynamicPredicate base. #1197
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
Travis is actually green (slow to report?)
07fdde0
to
6eebf29
Compare
Output is kept the same temporarily
6eebf29
to
c028a9a
Compare
Rebased, just to be sure. @JonRowe could this be merged please? Two subsequent PRs are waiting on this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay, after having had the time to finally give this a proper review, I have two questions.
def predicate_method_name | ||
predicate | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this should be in the Has
class as this implementation is only used there? (I've checked the other PR so I think thats the case?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah actually, I see, its not used elsewhere in has, ignore this comment. Commented on the wrong comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Answered my own question and the remaining one is not a blocker.
The
has_...
andbe_...
dynamic predicate matchers have a lot in common.This PR refactors both in terms of a common base class.
Output is kept the same temporarily
Precursor to #1195