Too lazy to fill this in.

Not sure how useful it's to list blacklisted items in satisfied_requirements. Same with TestCaseNotApplicable class.

Why remove this conditional?

Oh, I see now. This is to be handled by test-case-not-applicable and the blacklist stuff.

Cool. The approach looks good in general. I guess I'm worried about merge conflicts with the api v2 change. Will take a look soon to see how close that is to resolution.

Hmm yeah. These examples are getting really unwieldy. I like examples, there is value in having something concrete instead of an abstract explanation of the thing... but I wonder if it might be time to drop the examples and replace them with proper docs of the response structure (#169).

Yeah you'll notice after PR#184 they no longer have identical constructors (nor should they) so this mix-in thing is not a good approach.

Personally I find that mixins tend to just make everything harder to understand by spreading the interface and implementation around even further...

I'm with Ralph, this looks nice but can we please wait for PR#184 to land and then have another crack at it after that.

@dcallagh I concur, mixins are kinda strange.

PR#184 should be merged ASAP (and before this) -- it's getting bigger every day :-o -- reviewing right now.

I finally rebased the changes.

This test seems weird.

You're asserting that the satisfied requirements in the response match an expected set of satisfied requirements. That makes sense.

But, you're building the expected set of satisfied requirements from satisfied requirements in the response.

It seems like, if we make a change that breaks the way these are returned, the test won't catch it. It will be a tautology.

Tests all pass locally to me.

I have a concern about that one test. Other than that, :+1:. Does the concern make sense @lholecek?

Changed.

Tests all pass locally to me.
I have a concern about that one test. Other than that, 👍. Does the concern make sense @lholecek?

Yeah, I don't know what I was thinking when I wrote the test but it was almost like comparing the response with itself. :)

I changed it.

LGTM. :+1: Merge when you're ready.

This won't work in Python 3 (dicts are no longer comparable). I had to fix one like it already.

It might make more sense grammatically if this were called 'missing-test-result-waived'. Either way is okay though.

Type for TestResultMissing is test-result-missing so I thought just adding suffix -waived would be more consistent with the naming.

Yeah I get that. But it was supposed to be almost like a sentence, or like newspaper-headline style.

'test-result-missing' = a test result was missing
'missing-test-result-waived' = a missing test result was waived
'test-result-missing-waived' = a test result was missing but its absence was waived?

So yeah I guess either way works.

I think, in Python 3, comparing dicts with == should work fine (unless the values cannot be compared with same operator). Do you have an example where the line above won't work?

Did you try running it in Python 3? :-)

The issue is the sorted() not the == comparison. Equality is of course fine. But dicts are not comparable.

>>> sorted([{}, {}])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: '<' not supported between instances of 'dict' and 'dict'

Although I do see that there is no error if you sort a single-element list. I am not sure how many results there are in this test case, perhaps only 1, which might be why it is not failing. However in that case, there is no reason for the sorting anyway...

Oh OK that makes sense - I tried only sorted([{}]) :). I'll remove the sorted if that will work.

OK, fixed. The test passed if I remove sorted() (though there are three items, I'm not sure if the order can be different in some cases).

@dcallagh OK to merge?

Oh, I didn't realise you'd updated it. Pagure doesn't send any notification when a PR is amended.

Yep LGTM :+1: