dataflake / Products.LDAPUserFolder

LDAP-enabled Zope user folder
https://productsldapuserfolder.readthedocs.io/
Other
2 stars 7 forks source link

fix tests #4

Closed drfho closed 2 years ago

drfho commented 4 years ago

Hi @dataflake, please let travis take a look at the test fixes: does it still break? Thank you Frank

drfho commented 4 years ago

sorry, there are some fxes missing.... will come...

dwt commented 4 years ago

I have picked up the work on this pull request, and can invest about half a day into it. Lets see how far I can get this.

dwt commented 4 years ago

I see lots of complaints of isort that must be part of master too - do I have a wrongly configured version of isort or is it ok to ignore these?

dataflake commented 4 years ago

I have fixed all isort and flake8 issues except for the one use of unicode. I also removed the [tox] stanza from the buildout configuration because if you install tox through the buildout it will not work for parallel runs. Re-run the buildout, which will remove bin/tox, and then reinstall tox using pip or use any other existing current tox script you already have.

dwt commented 4 years ago

Thanks!

dwt commented 4 years ago

Is it ok to remove help registration, as it is not supported in zope 4 and 5 anymore?

dataflake commented 4 years ago

Instead of slowing yourself down by asking and waiting for a response just continue doing what you think is right. Anything can and will be corrected when it's time to review the PR.

dwt commented 4 years ago

From taking an initial look, it seems that python-ldap has pretty much inverted their own stance on how to handle unicode in python when switching to version 3, and especially since version 3.3 (py3 only) see python-ldap documentation

I.e. at some point in the past you could choose what was text and what was binary, and they accepted binary in most places. but today all arguments seem to have to be text (py3:str). On cursory reading this seems completely contrary to what Product.LDAPUserFolder and dataflake.fakeldap try to enforce. Not sure I get this correctly (here I'd really like some input / rationale if possible), but it seems that the library wants to enforce that all arguments are converted to binary (py3:bytes, py2:str) before passing them to the library?

Am I getting this wrong?

Becaus if not, porting this will be a bit annoying, because it basically either requirers a complete makeover of dataflake.fakeldap (and I have no clue if you are using that in other places too) or radically different behaviour of the mocks on python3.

I like that text/binary handling is quite explicit in Products.LDAPUserFolder, which should make it fairly predictably and simple to change this - even if a bit annoying. Not sure if code using this stuff will have to change, as it basically just provides a zope compatible user and user-folder which only needs to conform to Zope's usage of text/binary.

Not sure Python2 compatibility is reasonably easy to achieve in this context, especially without switching over to the new way of handling text/binary from python-ldap - and especially since I don't quite get if there are other users of the library that will need to adapt.

So, did I get the idea of the library correct? Are you using dataflake.fakeldap anywhere else? Can I just change it to suit the new interfaces?

Anyway, since I won't be working on this on the weekend probably, I thought I could also give talking a try to get some progress.

dataflake commented 4 years ago

Sorry for not responding earlier, got too much to do right now.

I have not looked at any of this in several years. It used to be the case that python-ldap or pyldap required incoming data as UTF8-encoded byte strings, because that's what the LDAP libraries themselves expect. So helpers like dataflake.fakeldap try to follow that convention. If that has changed and the Python bindings now want unicode/Python 2 and native strings/Python 3 then that's indeed a big change.

By the way, I think very early on I had mentioned that it would make most sense to start with dependencies like dataflake.fakeldap so the tests here can run, and then continue fixing here.

dataflake commented 2 years ago

This is obsolete, a fixed version has been released.