PatternMatcher / Matcher

[Stephane Bailliez <sb...@gmail.com>]

I'm looking at the source code of this package.

What is supposed to be the contract of:
- PatterMatcher.getMatcher(String expression)
- Matcher.matches(String input)

In both cases I see absolutely no reason to accept a null argument.
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.

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


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.
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 )

Thanks !

-- stephane

Re: PatternMatcher / Matcher

[Stephane Bailliez <sb...@gmail.com>]

Xavier Hanin wrote:
>
> 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.

Ok, thanks. This will end up in the doc.

-- stephane

Re: PatternMatcher / Matcher

[Xavier Hanin <xa...@gmail.com>]

On 1/6/07, Stephane Bailliez <sb...@gmail.com> 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
>