Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Python 2 / 3 compatible imports. #10

Merged
merged 3 commits into from
Jan 22, 2018
Merged

Python 2 / 3 compatible imports. #10

merged 3 commits into from
Jan 22, 2018

Conversation

rudaporto
Copy link
Contributor

No description provided.

@rudaporto rudaporto self-assigned this Oct 22, 2017
@rudaporto
Copy link
Contributor Author

@pbauer can you help me with this review or should I ask someone else?

Copy link

@tomgross tomgross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!
Is six a dependency in setup.py already?
A changelog entry is missing.

@rudaporto
Copy link
Contributor Author

rudaporto commented Oct 25, 2017

@tomgross I update with more compatible changes.

  • remove some unused imports of urllib.quote_plus
  • change to use built-in set class when sets (deprecated) module is not available
  • add six.text_type instead of unicode

Still, there is some unused imports but I did not removed them at this stage.

@rudaporto
Copy link
Contributor Author

@tomgross should I change travis.yaml to use tox run the build with python 2.7, 3.4, 3.5 and 3.6?

from sets import Set as set
except ImportError:
pass

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just remove the import and always use the built-in set type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@tomgross
Copy link

tomgross commented Nov 1, 2017

@rudaporto This package depends on GenericSetup (Python 3 upgrade is WIP) and ZServer (This package will probably never see Python 3)

To get an overview what is achieved it is probably a good idea to have them with allow_failures option on.
I don't know, if there is an official statement on Python 3 subversion compatibility but I'd focus on 3.5 and 3.6.

@tomgross
Copy link

tomgross commented Nov 1, 2017

Maybe it is a good idea to have coveralls run too to see where the blind spots are.

@tomgross
Copy link

tomgross commented Nov 1, 2017

Apart from that this LGTM.

@tseaver
Copy link
Member

tseaver commented Nov 1, 2017

We should make the dependency on ZServer optional. The dependency is there to support detection of an FTPRequest, but if ZServer is not installed, we can just create a no-op class, e.g.:

# in Products/PluggableAuthService/plugins/RequestTypeSniffer.py
try:
    from ZServer.FTPRequest import FTPReques
except ImportError:
    class FTPRequest(object):
        """Dummy for missing ZServer's 'FTPRequest'."""

@icemac
Copy link
Member

icemac commented Nov 3, 2017

@tseaver +1 to make ZServer optional, some weeks ago I created #8 to address this issue. Maybe it can be done in another PR.

Copy link
Member

@icemac icemac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@icemac
Copy link
Member

icemac commented Dec 8, 2017

What has still to be done to get this PR merged?

@rudaporto rudaporto merged commit 2209ff9 into master Jan 22, 2018
@icemac icemac deleted the py3_imports branch January 23, 2018 06:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants