On 1/6/07, Stephane Bailliez <[EMAIL PROTECTED]> wrote:
I'm looking at the source code of this package. What is supposed to be the contract of: - PatterMatcher.getMatcher(String expression)
The contract is to return a Matcher implementation constructed with the given expression - Matcher.matches(String input) The contract is to return true if the input "match" with the matcher, false otherwise. In both cases I see absolutely no reason to accept a null argument. I must admit I don't know why the code tries to deal with null in some cases, but not consistently. It seems that what is done is that if the expression is null then a null input will be considered matching. At least it's the case in Exact and in a method of RegexpPatternMatcher which is no longer used (matches(String, String)). So I agree that having a clean contract saying that both methods throw an exception on null would be better. As far as I can read implementation differs regarding the acceptance of
null. Some will deal with it and go the extra step to compare null to null, some will blow up. GlobPatternMatcher goes the extra work of not blowing up if the pattern syntax is invalid (but logging a message) and will later if the compiled pattern is null returns true if the input is null too. That's pretty wild and you can be sure that it will do the wrong thing and no one will notice.
You're right, error management is definitly not very good to say the least in Ivy, and you pinpoint one problem here. So it's not consistent and needs to be fixed so that it is more clear.
Do you agree that: - The GlobPattern MUST failed (just like the regexp one) if the pattern syntax is invalid. - The expression must not be null. - The input must not be null
+1 Don't bother to fix anything, I will be hopefully cleaning some code,
documenting it (javadoc) and writing testcases. And I'd like to remove this getInstance that is really annoying too. Bonus question: Matcher.isExact(). While I understand that it is to figure if a match is an exact match (vs a partial match)... I failed to see the reason that motivated this method.
Well, it's not really to say if it isn't a partial match, it is to say if the matcher matches *only* if the expression equals the input. This is used only as a performance trick, to avoid scanning for things when you already know exactly what you want. In the install task where it used it avoid scanning the repository to list all modules to find that only one matches, and that it has the name requested. If I want to be nasty I could write a regexp which is an exact match
while the matcher will return that is not an exact match (because in practice you only know if a matcher is exact only after doing it). and that's the same for the ExactOrRegexp which can do an exact match but it says it is not. I see that the isExact() is actually only used once. For install(). And I'm not exactly sure to understand what implication it has (I could go deeply in the code but think a short explanation of the rationale behind it is certainly more interesting )
Sure, I hope my short explanation above will help. - Xavier Thanks !
-- stephane
