Code review comment for lp:~mbp/subunit/408821-filter-re

> A few thoughts. Broadly this is fine and I like the feature.
>
> Rather than document in README, perhaps extending the help (module
> docstring) of subunit-filter would be more accessible and provide a home
> for other such one-command things. OTOH we probably need a manual at
> some point anyway, so up to you.

I think you need some overall "here's what you can do with it" documentation, so I put it there.

>
> Rather than --[anti]-match, I'd rather something like:
> --with RE
> --without RE

ok

>
> Related to that, I wonder if without is really needed - the (?!...) can
> embed negative patterns. I'm fine with it, just speculating.

jeff's mum will never guess that.

> In the internals though, I think it would make sense to have a single
> re:
> patterns = []
> for pattern in options.with_patterns:
> patterns.append(pattern)
> for pattern in options.without_patterns:
> patterns.append("^(?!%s)$" % pattern)
> rule = "|".join(patterns)

I think it's much more useful to have (A AND NOT B) than (A OR NOT B) as far as cutting down what's present in the output.

> The interface is a little awkward - you use a functional style, but it
> doesn't fit all that well. For instance - "str(None) -> 'None'".

That seems like a non sequiter.

And I guess I see subunit as generally wanting to be functional.

> So,
> your code will incorrectly match on a pattern like 'one'.than

I don't think so, unless the test can be None.

>
> I think I'd prefer it if, in the Filter result you did:
> should_filter = self._check_predicates(test, reason|error)
>
> and that function (not a lambda) does
> if not callable(self._filter_predicate):
> return False
> else:
> if info is None:
> info = ''
> else:
> info = str(info)
> return self._filter_predicate(test, info)

1- Python sucks :-/, if I wanted 'if (a!=NULL)' everywhere I'd use C :-}

2- I think it's useful to allow future code to provide filters that look at objects not strings, seeing as the rest of it deals with objects. I realize they will mostly be lame objects that just wrap strings, so perhaps it's not worth it. If I was looking at bzr errors in-process I might want to poke through their attributes. For instance you might want to pass isinstance(err, KnownFailure).

3- Doing nothing if an object is not the type you expect is pretty error prone.

4- Looking at it now I'd like to fold *all* the filtering into evaluation of a predicate, so that this code was just changing one method not ~five. But that's a larger change and ties to your thing of how the status should be handled.

Sorry if this sounds grumpy. Thanks for the fast review.

> And that way folk writing predicates can be sure they are dealing with
> strings - we centralise that knowledge.
>
> Lastly, and this can come later if you like, I'd really like the ability
> to specify multiple patterns, which optparse can do quite easily with
> store="append".

Agree, but later.