Add Pyramid 2.0 compatibility

[msander]

• replace pyramid.compat by six
• replace set_request_property by add_request_method
• fix flake8 issues

fixes #32, #26 and #28

[digitalresistor]

Pyramid 2.0 is strictly Python 3, replacing pyramid.compat with six seems a bit silly. In most cases we can rely on b prefixes where necessary.

[stevepiercy]

@bertjwregeer should we drop EOLed Pythons and add supported versions, as well as move this to GitHub Actions?

[digitalresistor]

Yes, this extension needs a lot of love as it has received none over the years, but just dropping Py2 support and matching Pyramid 2.0 would be a good start.

[stevepiercy]

@bertjwregeer how should we proceed? We could do it all in one swoop—fixing bugs, dropping EOLed Pythons, moving to GHA, and adding Pyramid 2.0 compat—or do it in baby steps to cut two or more releases. I can do the work, just need advice and direction.

[msander]

Pyramid 2.0 is strictly Python 3, replacing pyramid.compat with six seems a bit silly. In most cases we can rely on b prefixes where necessary.

Pyramid 2.0 compatibility does not mean that you need to drop Pyramid 1.x compatibility (which supports Python 2).
However, Python 2 is EOL and I have not touched any Python 2 projects in years. So I guess it would be a reasonable decision.
If you want, I can modify my pull request and remove six.

[stevepiercy]

Thank you for the docstring changes!

[msander]

Thank you for the docstring changes!

Thank you for you review.

However, PEP8 is failing again due to these changes:

pyramid_ldap/__init__.py:132:42: W605 invalid escape sequence '\e'
pyramid_ldap/__init__.py:133:45: W605 invalid escape sequence '\`'
pyramid_ldap/__init__.py:137:15: W605 invalid escape sequence '\e'

Don't we need double backslashes here?

[stevepiercy]

Flake8 gives a false positive. This is a reStructuredText inline literal, but Flake8 does not interpret it as such. See screenshot for how it renders to HTML with the changes I requested and which you applied.

I could not find in PEP8 that it covers this edge case. To avoid the Flake8 warning, change the last line of the docstring to ignore this warning:

"""  # noqa: W605

Also I missed a couple of changes in tests.py that should have the same # noqa: W605:

pyramid_ldap/tests.py:206:31: W605 invalid escape sequence '\l'
pyramid_ldap/tests.py:216:40: W605 invalid escape sequence '\*'

To avoid the warning, pass tests, and not obscure the original intent of the test, append a similar comment:

        inst.authenticate('BAD\login', 'password')  # noqa: W605
#...
        inst.authenticate('login', 'bad\*()password')  # noqa: W605

[msander]

Thank you for your explanation.
Nevertheless, I think the double backslash is the correct solution.
The alternative would be to use a raw string (r"...").
An invalid escape like "\*" is currently interpreted as "\\*", but will result in a SyntaxError in the future. That's what the warning is about.

https://www.flake8rules.com/rules/W605.html

Also the tests are correct and finally pass with the double backslash.

The documentation is also rendered the same.

[stevepiercy]

OK, thank you for explaining. I misunderstood. I apologize for my mistake.

We have two failed build jobs, one of which is an allowed failure for Python 3.7 and the other for a false positive that coverage is below 100%. We can skip the Python 3.7 failure for now, but we need to resolve the coverage issue. I've seen this issue in other repos. I'll look for how we fixed it. It was working just prior to this PR.

[stevepiercy]

When I run tox -r -e py2-cover,py3-cover,coverage locally, it passes. I checked closely in the Travis log, and noticed "Cache entry deserialization failed, entry ignored". I cleared the cache, and restarted the build. It still fails. I have no clue how to fix this, other than try moving it to GitHub Actions and forget about Travis.

[msander]

Tests are passing now.

@bertjwregeer in setup.py, there is a comment 'six', # required by `ldappool` but not in their requirements file
This seems still to be true and I think we should keep that requirement. Do you still want to remove the usage?

[stevepiercy]

Tests are passing now.

@bertjwregeer in setup.py, there is a comment 'six', # required by `ldappool` but not in their requirements file
This seems still to be true and I think we should keep that requirement. Do you still want to remove the usage?

I see no instances of six in its code, so maybe it is OK to remove? Please try and let's see what happens.

[msander]

Please take a look at
https://opendev.org/openstack/ldappool/src/branch/master/ldappool/__init__.py

[digitalresistor]

Please take a look at
https://opendev.org/openstack/ldappool/src/branch/master/ldappool/__init__.py

I missed this... so then yes, keep the requirement for six.

[stevepiercy]

@bertjwregeer I think this is OK to merge, unless you have any further review.

I can work on bringing it to Pylons Project standards and parity with Pyramid 2.0 for Python support and GHA later this week.