[purs ide] Represent Filters as a datatype rather than functions#3547
Conversation
This allows us to do some reordering and optimizing on the order in which filters are applied
hdgarrood
left a comment
There was a problem hiding this comment.
LGTM :) I don't have any objection to breaking the protocol if nobody is using it. I just have a couple of small comments; totally up to you if/how you address them.
| Filters are functions of type =Map ModuleName [IdeDeclaration] -> Map | ||
| ModuleName [IdeDeclaration]=. We keep the =Map= structure around to make the | ||
| common case of filtering by module names fast. | ||
| Filters are functions of type =Map ModuleName [IdeDeclaration] -> |
There was a problem hiding this comment.
Perhaps this is a bit pedantic of me, but I think it's probably safer to say that filters can be thought of as functions of that type, rather than actually being functions of that type. I think there are lots of functions of that type which aren't expressible as filters, or which wouldn't be commutative.
| ( Filter | ||
| , declarationTypeFilter | ||
| , namespaceFilter | ||
| ( Filter(..) |
There was a problem hiding this comment.
Unless I'm missing something I think you can continue not to export the constructor?
|
|
||
| newtype Filter = Filter (Endo [Module]) | ||
| deriving (Semigroup, Monoid) | ||
| newtype Filter = Filter (Either (Set P.ModuleName) DeclarationFilter) |
There was a problem hiding this comment.
Have you considered having Filter be a newtype around [Either (Set P.ModuleName) DeclarationFilter]? Then I think you could still have a monoid instance which does something similar to optimizeFilters. Just a suggestion; it might be nice for consumers of this API not to have to worry about the difference between a single filter and a collection of filters.
There was a problem hiding this comment.
I did, I just wanted to keep the model closer to the protocol here. I'm thinking about making a new command which makes more assumptions about how editors end up asking for completions which could change things around a bit more.
|
Thanks for reviewing! |
This allows us to reorder the filtering to always filter by module name first and exploit the fact that we store our declarations in a Map of a module names.
Also cleans up the declaration type filter implementation (I don't know of any editor plugin that was using these so far so I decided to break the protocol). If I'm wrong please let me know.