Set custom prefix filter on predictor #100
Conversation
Codecov Report
@@ Coverage Diff @@
## master #100 +/- ##
========================================
+ Coverage 89.76% 90% +0.23%
========================================
Files 12 13 +1
Lines 879 900 +21
========================================
+ Hits 789 810 +21
Misses 79 79
Partials 11 11
Continue to review full report at Codecov.
|
| } | ||
|
|
||
| //PrefixFilteringPredictor is a Predictor that also implements PrefixFilter | ||
| type PrefixFilteringPredictor struct { |
There was a problem hiding this comment.
Maybe this struct definition is not necessary?
If anyone wants to use the new interface, They will just make sure that the predictor also implements the new interface?
There was a problem hiding this comment.
I think this will be useful in instances where somebody wants to use an existing Predictor with a non-default PrefixFilter.
An example would be if I want to use a SetPredictor with a case insensitive matcher I would do something like:
&complete.PrefixFilteringPredictor{
PrefixFilterFunc: complete.CaseInsensitivePrefixFilter,
Predictor: complete.PredictSet("foo", "bar", "baz", )
}Without it, I end up implementing PrefixFilteringPredictor in my own codebase. Admittedly it's pretty simple to implement though.
There was a problem hiding this comment.
Following https://github.com/posener/complete/pull/100/files#r301385506, this won't be needed.
| } | ||
|
|
||
| //PermissivePrefixFilter always returns true | ||
| func PermissivePrefixFilter(_, _ string) bool { |
There was a problem hiding this comment.
MatchAny?
Maybe this is also not needed?
| } | ||
|
|
||
| //CaseInsensitivePrefixFilter ignores case differences between the prefix and tested string | ||
| func CaseInsensitivePrefixFilter(s, prefix string) bool { |
There was a problem hiding this comment.
Suggestion: Consider a different API where this is a factory method that accepts a Predictor and returns a MatchPredictor: it attributes it with case insensitivity.
WDYT?
There was a problem hiding this comment.
I think I like that suggestion if I understand it correctly. I'm assuming that MatchPredictor would be an interface that includes both Predictor and PrefixFilter (that brings up a naming question which I'm ignoring in this comment to stay focused on this part).
I'll work on an API change that looks like that.
There was a problem hiding this comment.
I think you understood correctly.
This will also enables us dropping PrefixFilteringPredictor.
|
Thanks for the comments. I am away at a company event this week, so I can’t make any updates immediately. I should be able to address the comments within a week. I just wanted to let you know so you don’t think this is an abandoned drive-by PR. |
|
@posener, Do you have a suggestion for a better name than PrefixFilter? It was the best name I could think of at the time without reusing any terms that are already in use in the project. I half expected your first bit of feedback to be "That name is awful, you should call it X". |
:-) I would go with |
This replaces #99. I didn't like that you had to change the filter for all arguments or nothing there. This allows the custom prefix filter to be set in the predictor.
It adds the
PrefixFilterinterface. When a predictor implements PrefixFilter, it will use the supplied filter instead of the defaultstrings.HasPrefix. A new typePrefixFilteringPredictoris also introduced. It wraps both a Predictor and a PrefixFilter.