Comments (9)
Hi Ryan,
Option 3 is certainly the best option. The approach that we've taken with the
helper
is to monkey patch the bits of various modules that need changing rather than
providing a completely new implementation.
The helper needs to support both the current stable release and trunk, in most
cases
so far this hasn't been too much extra work to achieve.
If that sounds reasonable to you we'd be happy to receive any patches you can
supply.
You'll need to sign the Google CLA at
http://code.google.com/legal/individual-cla-v1.0.html before we can accept any
code.
Thanks.
Original comment by [email protected]
on 15 Apr 2008 at 1:19
- Changed state: Accepted
from google-app-engine-django.
Hi Matt - Here is a patch that should enable the traditional django email api
for
both 0.96 and 0.97+. This should be the only minor dependency that was holding
back
inclusion of CommonMiddleware.
Turns out that the /django/core/mail.py code changed considerably, so I had to
implement the functionality two different ways. As we move to 0.97 we should
be able
to trim the new appengine_django/mail.py file.
Let me know if you have any questions.
Original comment by [email protected]
on 15 Apr 2008 at 10:33
Attachments:
from google-app-engine-django.
Hi Ryan,
Thanks for your work on this. It looks good, a few minor comments:
* You monkey patch the revised methods in both mail.py and __init__.py, please
confine all monkey patching to the Install... method that you create within
__init__.py.
* Once you do this, you can remove the if VERSION statements from mail.py and
leave
it up to the Install method to decide which implementation to monkey patch into
use.
* Please ensure that there are no lines over 80 characters in length.
* The docstring for send_mass_mail looks to be copy/pasted from Django and
doesn't
match the implemenation (eg. EMAIL_HOST_USER and EMAIL_HOST_PASSWORD are not
actually
used), Please fix this comment to match the actual behaviour.
* Please re-order and group the import statements so they are alpabetically
listed,
with standard Python imports first, followed by django imports and imports from
google.appengine in the final group.
Finally, do you have any tests that we can use to verify that this code works
correctly in future?
Thanks.
Original comment by [email protected]
on 22 Apr 2008 at 5:35
- Changed state: Started
from google-app-engine-django.
Updated Diff based on comments from Matt. Note that I didnt want to stick the
GoogleSMPTConnection class in __init__.py, so I need to at least do a check in
the
mail.py file for the existence of its parent (otherwise an exception is
thrown). If
the parent class does not exist, we are in 0.96 and we dont want to create the
class.
One alternative to this check would be to define two different .py files -one that
is loaded in 0.96 and the other in 0.97+. (ick) I'm new to python though, so if
there is a better way to handle this let me know.
I dont have unit tests for this right now. Will have to work on those later. I
tested on a sample app that I had been working on (and had seen the original
issue).
Original comment by [email protected]
on 22 Apr 2008 at 3:14
Attachments:
from google-app-engine-django.
You can use an idiom such as the following to deal with parent class differences
between 0.96 and 0.97:
try:
# 0.97 path
from django.core.mail import SMTPConnection
except ImportError
# 0.96 path
SMTPConnection = object
class GoogleSMTPConnection(SMTPConnection)
Obviously this will result in a class that doesn't have all the required
functionality when running under 0.96, but that's OK, because the crippled
class will
never be monkey patched into place.
See appengine_django/db/base.py for an example of this in practice.
Lines 67, 103, 110 and 117 of your mail.py still contain monkey patching code
that
should be moved into the Install function.
Original comment by [email protected]
on 23 Apr 2008 at 4:35
from google-app-engine-django.
I think this should do the trick. All version info moved out of mail.py as
well as
all references to overwriting core django code.
Original comment by [email protected]
on 23 Apr 2008 at 6:12
Attachments:
from google-app-engine-django.
Hi Ryan,
The monkey patching code looks good now.
* Please add copyright and Apache 2.0 license headers to mail.py.
* The warnings on lines 44-50 of mail.py are still > 80 characters.
* In general I think lots of the logging could be reduced, it seems very
verbose.
A few more comments on the implementation, now that I've studied how Django
does it
more closely, sorry for not picking these up in the initial review:
* Why do you need to replace mail_admins and mail_managers?
* For the Django 0.97 SMTPConnection class, you could re-implement just the
_send
method with faked open and closed methods that just ensure that the .connection
member is True. Then you don't have to replace the send_messages method which is
where all the logic is. This will make the class easier to maintain in future
and
will retain the ability to send multi-part messages.
* For Django 0.96 you could provide a replacement implementation of smtplib.SMTP
instead of modifying the send_mass_mail method. This would preserve the ability
to
send multi-part messages.
Original comment by [email protected]
on 28 Apr 2008 at 12:52
- Changed title: Replace django mail functions with app engine compatible versions
from google-app-engine-django.
* Please add copyright and Apache 2.0 license headers to mail.py. (Done)
* The warnings on lines 44-50 of mail.py are still > 80 characters. (Done)
* In general I think lots of the logging could be reduced, it seems very
verbose.
(Removed some, moved others to logging.info...)
* Why do you need to replace mail_admins and mail_managers? (I created a new
method,
called mail_group, to add some additional functionality. This allows you to use
either the settings.ADMINS or MANAGERS if defined. If not, it defaults to the
Google
App Engine admin group.)
* For the Django 0.97 SMTPConnection class, you could re-implement just the
_send
method with faked open and closed methods that just ensure that the .connection
member is True. Then you don't have to replace the send_messages method which is
where all the logic is. This will make the class easier to maintain in future
and
will retain the ability to send multi-part messages. (Ok, did this but it just
moves
the logic to another method. Guess it does decouple from the counting logic
though
so that is nice...)
* For Django 0.96 you could provide a replacement implementation of smtplib.SMTP
instead of modifying the send_mass_mail method. This would preserve the ability
to
send multi-part messages. (Unclear what you mean here - you mean the
attachments?
Doesnt look like 0.96 supported attachments at all - the api doesnt have the
ability
to specify them. I could be wrong. Also, email attachment conversion is
outside of
the scope of this patch.)
Apologies for the delay in turning this around.
Original comment by [email protected]
on 6 May 2008 at 7:17
Attachments:
from google-app-engine-django.
Thanks Ryan,
I've committed this code in r16.
Original comment by [email protected]
on 8 May 2008 at 12:52
- Changed state: Fixed
from google-app-engine-django.
Related Issues (20)
- Appengine docs are not consistant HOT 1
- support installing with an exact version to avoid UnacceptableVersionError HOT 2
- Allow non-django handlers to mix with django handlers in the same app HOT 4
- ImportError: No module named fancy_urllib HOT 6
- Wrong value of appid and have_appserver, HOT 2
- Default Database not set and objct User dont have attribute 'properties' HOT 2
- Cannot update my application remotely HOT 1
- simplejson importerror HOT 2
- ImportError: No module named whoosh (needs path update for SDK 1.5.3) HOT 4
- BadRequestError: app "google-app-engine-django" cannot access app "MY_APP"'s data in IntegrationTest.testBasic HOT 4
- Adds upgrade script to keep an application in sync with the helper
- console management command doesn't work
- AttributeError: 'module' object has no attribute 'ApplicationLoggingHandler'
- Since GAE1.6 - ValueError: too many values to unpack in LoadAppengineEnvironment HOT 5
- SDK 1.6 LoadAppConfig ValueError: too many values to unpack HOT 1
- pls help me out....while browsing project i got this error ..!!!!
- from google.appengine.dist works in local 2.7 environment HOT 3
- ImportError: cannot import name SMTPConnection HOT 1
- Goflow implementation in google app engine nosql.
- Enter one-line summary
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from google-app-engine-django.