Merge ~quantifics/django-saml2-idp/+git/stable:py3 into ~ubuntuone-pqm-team/django-saml2-idp/+git/stable:master

I also have a tox addition with flake8 fixes in the works, if you think that this can fit in the existing MP let me know and I'll update it

Would be better to add the information you placed at 'description of the change' in the commit message, as the description will get lost after committing and it is really useful.

Left some comments, but I leave it to someone else to confirm it makes sense what I wrote

One question and one comment/request to add a test. (Not Approving yet because I think the test might be interesting to have).

I added some tests and fixed some more issues plus added the tox.ini file. A few notes:

- I haven't tested these changes on the identity-provider yet. While this codebase now boasts a 96% coverage, I'd still like to run the identity-provider tests as well with a wheel file of these changes. I'll give a ping when I have a green light from those tests as well
- Flake8 in tox doesn't pass due to changes needed in various places in terms of empty lines needed, line length restrictions etc. I decided to leave it like this because adding those changes in the mix as well would make the MP huge and full of noise. So I thought that it would be better to run black and fix the flake8 issues in a separate MP
- I tried to add more tests in some files to reach a closer to 100% coverage but there were cases of functions that were simply not used [1] in the codebase. I decided to leave these and not remove them again to avoid juggling with too many things in one MP

[1] get_config_for_acs in saml2idp/metadata.py for example. I didn't check if this function is in some mysterious way used in the identity-provider, so I decided not to purge it

> I'd still like to run the identity-provider tests as well with a wheel file of these changes. I'll give a ping when I have a green light from those tests as well

I just finished running the SSO tests with a version of this MP and they pass (had to do a change in importing some test variables in SSO).

Looks amazing to me.

Adding a reminder to implement Wouter's suggestion above, to move the text from 'description of change' (which is only displayed on this MP page in launchpad) to 'commit message' instead, which forms the git commit message.

Also, FYI, a rule of thumb is to try and keep MPs less than 500 lines (as measured here on this page, not by 'diff'). It isn't a hard requirement, we can always discuss whether it makes sense for a particular MP, but I have seen people request that a large, hard-to-review MP be split up before it can get approved. I'm not suggesting that you have to do it here, just bear it in mind.

Incidentally, just for my understanding, do you think adding the flake8 changes to this MP would generate lots more noise because the code wasn't flake8 compliant before this MP? Or would flake8 only modify the code that you have added/changed, so actually wouldn't increase this MPs linecount after all?

@Jonathan, I normally agree with your suggestions about line count, but looking to the whole diff there are many lines causing this by proper typing them. So that probably could be split in a separate MR, but not sure it is worth the effort.

@John thank you for commenting the PyOpenSSL callback, commenting functions should happen way more often imho

> Adding a reminder to implement Wouter's suggestion above, to move the text from 'description of change' (which is only displayed on this MP page in launchpad) to 'commit message' instead, which forms the git commit message.

I'm copying the message I wrote on mattermost (~snap-store reviews) since I'm not sure which of the two is the best place to discuss this:
"""Regarding the suggestion of putting the description of a MP to the commit message: wouldn't that make the git log bigger? I know it may sound pedantic but usually the commit message is supposed to be brief (and I like it given that I usually see the log graph with the messages only)"""

> Also, FYI, a rule of thumb is to try and keep MPs less than 500 lines (as measured here on this page, not by 'diff').

I agree with that rule of thumb and can definitely understand why it is useful. The reason for the over-the-limit lines is because there was a request to add a couple of tests for the parts I had fixed and had added. And because these involve having certificates and saml requests as string variables, that resulted in a small avalanche of changes :/

> Incidentally, just for my understanding, do you think adding the flake8 changes to this MP would generate lots more noise because the code wasn't flake8 compliant before this MP? Or would flake8 only modify the code that you have added/changed, so actually wouldn't increase this MPs linecount after all?

It would generate lots more noise because the initial code base was not flake8 compliant and there are lots of empty lines and line breaks that need to be added. Had I added black to the mix we would also see a lot of double-instead-of-single quote changes

> @John thank you for commenting the PyOpenSSL callback, commenting functions
> should happen way more often imho

I figured it would be of big use here because m2crypto's passphrase callback function behaves differently. And I'm sad to report that initially I was lazy enough to use that instead of creating a new one. So the request for having this function tested as well got me digging up pyOpenSSL's documentation about what the structure of the passphrase callback should be.

And I agree on adding useful documentation in functions. Useful meaning "why this decision was taken here" instead of simply explaining what the function does. Functionality should be easily deductible by reading the code (if it's not, we need to make the code simpler). Rationale is what needs to be documented because it's not always deductible by reading the code

@Wouter "could be split in a separate MR, but not sure it is worth the effort.":
Agreed, thanks for the thoughts.

@John "The reason for the over-the-limit lines is because...":
I figured something like that. Fair enough!

@John "It would generate lots more noise because the initial code base was not flake8 compliant":
I see, thanks. Definitely agree with your decision not to do that in this MP.

I'm going to approve this to be merged, then. Also:

@John "I usually see the log graph with the messages only"
I know everyone has their own favorite versions of this, so ignore these if you prefer, but just in case, I use a lot of:

$ type -a gloga
gloga is a function
gloga ()
{
    glog --all "$@"
}

$ type -a glog
glog is a function
glog ()
{
    git log --graph --format=format:"%x09%C(yellow)%h%C(reset) %C(green)%ai%x08%x08%x08%x08%x08%x08%C(reset) %C(white)%an%C(reset)%C(auto)%d%C(reset)%n%x09%C(dim white)%s%C(reset)" --abbrev-commit "$@";
    echo
}

Thanks for the latest round of changes, looks OK to me!