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

WIP: Port Products.PrintingMailHost to Python3 and Zope 4 #6

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@pbauer
Member

pbauer commented Oct 3, 2018

No description provided.

@pbauer

This comment has been minimized.

Member

pbauer commented Oct 3, 2018

This needs to be checked if it still works with py2 and also with Zope 2.

@pbauer pbauer changed the title from WIP: Support for Python3 and Zope 4 to WIP: Port Products.PrintingMailHost to Python3 and Zope 4 Oct 3, 2018

@pbauer pbauer requested a review from frisi Oct 3, 2018

from AccessControl import ClassSecurityInfo
from Products.PrintingMailHost import LOG, FIXED_ADDRESS
from base64 import decodestring
from email.message import Message

This comment has been minimized.

@mauritsvanrees

mauritsvanrees Oct 3, 2018

Member

This email.Message import will fail on Python 2.4. That is what the except ImportError was for.
This is fine of course, but it deserves a note in the changelog, and the Python 2.4 and Plone 3.3 classifiers should be removed.

This comment has been minimized.

@pbauer

pbauer Oct 4, 2018

Member

👍

@frisi

maurits already mentioned valid points about 2.4 compatibility.
i'd say we you can remove the DevelopmentMode variable in addition.
LGTM otherwhise, thanks!

LOG.warning("Hold on to your hats folks, I'm a-patchin'")
import Patch
DevelopmentMode = True # noqa

This comment has been minimized.

@frisi

frisi Oct 4, 2018

Member

setting DevelopmentMode to True here might be misleading since it is also sufficient to set the environment variable ENABLE_PRINTING_MAILHOST to True to enter this condition.

i'd rather skip this here. afaict the variable DevelopmentMode is not used in this package at all.
if other packages depend on that they'd need to be fixed.

This comment has been minimized.

@mauritsvanrees

mauritsvanrees Oct 4, 2018

Member

Agreed, in current master we only import DevelopmentMode to check it once at startup for true/false, but we do not need it afterwards. So the variable can be removed.
No need to keep it for backwards compatibility, I would say.

@@ -7,6 +9,7 @@
FIXED_ADDRESS = []
TRUISMS = ['yes', 'y', 'true', 'on']
DevelopmentMode = False

This comment has been minimized.

@frisi

frisi Oct 4, 2018

Member

i guess we can remove that, see other comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment