You shouldn't need to add the "None" results. Just collect the exceptions and then use the number of rules in the policy - the failures to determine how many succeeded.

I recommend documenting this API.

@jcline, in @dcallagh's original posting he asks:

is this what you had in mind

.. and you added some inline comments in the pull request, but I'm not sure what to do next. This PR is in limbo!

Is it what you had in mind (roughly)? Should we work on it? Or abandon it?

@ralph, ah, I didn't realize this was blocked on me. It's pretty much what I had in mind, I just don't see any reason to store or track the passing rules with None.

Yeah it's really blocked on me. :-) I will try posting another version that doesn't use the Nones and see how that looks.

So the problem with taking out the Nones and counting rules in the policy is that the summarize() function is aggregating over potentially multple policies and potentially multiple packages. And in future (if we add per-package policies) we have to handle the case where the set of requirements for each package-policy combination may be different.

So the reason why the Answer approach works well is because, no matter which policies and which rules apply to which packages, we just collect a long list of Answers and then summarize them.

If we collect unsatisfied requirements (exception instances) and then compare them against the policy rules, the summarize() code has to do the same work again to figure out what the rules were for each item.

I'll post a patch which hopefully shows what I mean here...

Amended version is commit 47e60d8.

It shows the two problems I was trying to explain above. The summarize() functions API is messier now (see how the unit test for it is way more complicated?). And it assumes that the total number of applicable rules is (number of items * number of rules in the policies) but that assumption would not be valid if we add support for per-package policies in future.

In that case we would probably have to lift the summarization logic out of its little function into the top level loop where we apply each policy... or change the .check() API to return a tuple of (int count of satisfied rules, list of unsatisfied rules). Which is effectively what we have already today with the Answer types. :-)

Okay, well I leave it up to you as to whether you find the summarize function less appealing than classes. If you opt for the classes, I highly recommend that you not use both a boolean attribute on the class as your inheritance structure renders it pointless (just do isinstance).

I do wonder why Greenwave is in the business of creating a human-readable string for its JSON API that doesn't contain any information a client couldn't easily create themselves. It seems like it would be better to provide all the information to the client and let them decide how to summarize it. It has the added advantage of letting you side-step the problem of translations.

What about using a class "TestResultPassed" instead of "None"? Instead of taking out "NONE", the list would be:
[TestResultPassed, RuleNotSatisfied(...), TestResultPassed, ...]
which feels clear to me.

Isn't that what we already have? :-)

Yeah, but we are not using exceptions for unsatisfied rules. Maybe I miss something here as I can not remember what your first commit looks like?

Okay but then it would be effectively the same as what we have now, but returning satisfied answers and raiseing unsatisfied ones (but then collecting them all up in a list anyway). I don't see that it makes the code much easier to understand that way.

Let's close this for now. We can revisit the idea later when Greenwave is a bit more fleshed out, or if any other cleaner ways of implementing it come to mind.