Suggestion: support multiple filters of the same type #1

Activity

1. kilahm commented on Aug 18, 2015

   

   kilahm

   on Aug 18, 2015

   Member

   More actions

   When adding multiple filters, in which way are they combined?

   I.e., is

   $builder->includeClasses()
   ->filterClasses($c ==> $c->getName() === 'SomeName')
   ->filterClasses($c ==> $c->getName() === 'OtherName')

   equivalent to

   $builder->includeClasses()
     ->filterClasses($c ==>
       $c->getName() === 'SomeName' ||
       $c->getName() === 'OtherName'
     )

   or

   $builder->includeClasses()
     ->filterClasses($c ==>
       $c->getName() === 'SomeName' &&
       $c->getName() === 'OtherName'
     )

   In theory I agree that being able to write really short filters and explicitly list each one individually would be nice. I think the user would have to decide what that combination means though. As it stands, simply adding two filters in series is fairly ambiguous. I think I might rewrite your example as

   (new ScannedDefinitionsYAMLBuilder($source, $parser, $destination))
     ->addFilter($x ==> 
       ScannedDefinitionFilters::IsHHSpecific($x)) ||
       !ScannedDefinitionFilters::ShouldNotDocument($x)
     )
     ->build();

   This way the user is forced to think about how the filters are composed into more complex filters.
2. kilahm commented on Aug 18, 2015

   

   kilahm

   on Aug 18, 2015

   Member

   More actions

   I originally started writing this library with that type of API (as you may recall from my email). But being able to handle both ways of composing multiple filters makes things a bit confusing.

   I mostly built this type of composition flexibility in a DB abstraction layer (that I haven't published). It looks something like:

   $db->fetch(CollectionName::SomeTable)->where(
     $model->field->is('blah')
     ->orWhere($model->otherField->lt(15))
     ->asWellAs($model->thirdField->in(Vector{'one', 'two'}))
   );

   That would render to and SQL statement something like

   SELECT * FROM `SomeTable` WHERE ( `field` = "blah" OR `otherField` < 15 ) AND ( `thirdField` IN ("one", "two") )

   Note that this is not allowed

   $db->fetch(CollectionName::SomeTable)->where(
     $model->field->is('blah')
     ->orWhere($model->otherField->lt(15))
     ->andWhere($model->thirdField->in(Vector{'one', 'two'}))
   );

   because the grouping is ambiguous.

   I really wanted a fluent api to allow arbitrary composition of logical AND and OR statements. It gets pretty hairy, but it can be done.
3. fredemmott commented on Aug 18, 2015

   

   fredemmott

   on Aug 18, 2015

   Author

   More actions

   I'm not aware of an API where adding a filter can increase the amount of data returned. && 'feels' common and matches expectations when using collecitions, eg $map->filter($a)->filter($b) == $map->filter($x ==> $a($x) && $b($x))
4. kilahm commented on Aug 18, 2015

   

   kilahm

   on Aug 18, 2015

   Member

   More actions

   Except then the only way you could compose the filters with an OR is to explicitly combine them in another function. That feels asymmetric to me.

   The use case would be: "I would like classes in namespace A and I would like classes in namespace B and I would not like classes in namespace A\Test". I could see attempting to translate that sentence into a chain of filter calls on the builder very similar, if not identical to, the chain you mention.
5. fredemmott commented on Aug 18, 2015

   

   fredemmott

   on Aug 18, 2015

   Author

   More actions

   IMO that doesn't match the meaning of the word 'filter'. if there were an 'include()' function I'd expect it to be an OR not an AND
6. kilahm commented on Aug 18, 2015

   

   kilahm

   on Aug 18, 2015

   Member

   More actions

   So something like this?

   $builder
   ->includeClassesFilteredBy($filterA)
   ->includeClassesFilteredBy($filterB)
   ->filterAllClasses($filterC)
   ->filterAllClasses($filderD)
7. fredemmott commented on Aug 18, 2015

   

   fredemmott

   on Aug 18, 2015

   Author

   More actions

   I'd be fine with that; though here's an alternative this would completely solve the 'filter' naming issue:

   ->onlyClassesMatching($filterC)
   ->onlyClassesMatching($filterD)
   

   As a side note, implementation shortcut: https://github.com/fredemmott/definitions-finder/blob/master/src/FileParser.php#L115-L122
8. kilahm commented on Aug 19, 2015

   

   kilahm

   on Aug 19, 2015

   Member

   More actions

   I think the word "match" is a little to narrow for generic filters. I also agree with your point about Map::filter being a good example of the usage of the word "filter".

   I would be OK having multiple filter calls always remove from the list now that we agree on having multiple include calls always adding to the list. Before, having only a single include and multiple filters, it was difficult to express "I want classes in namespace A and classes in namespace B" by chaining simple filters.

   I could also see having methods like includeAllClasses() which would be equivalent to includeClasses($c ==> true). Similarly, have filterAllClasses() equivalent to filterClasses($c ==> false).