replace djangoforms with manual code, in preparation for py27 port

replace djangoforms with manual code, in preparation for py27 port

[GvR, 2012-01-27 03:12:05]

This isn't pretty, but the Python 2.7 runtime doesn't support djangoforms, so I
figured I'd give it a shot.

The py27 code isn't ready for prime time yet, but you can try it out at
http://py27.codereview-hr.appspot.com.  I've tested these operations:

- create repo
- add branch
- edit branch
- delete branch
- errors in each of the forms

[GvR, 2012-01-27 03:32:33]

FWIW, to see this in context: http://codereview.appspot.com/5574079/

[Andi Albrecht, 2012-01-27 06:26:30]

http://py27.codereview-hr.appspot.com/5552045/diff/1/codereview/views.py
File codereview/views.py (right):

http://py27.codereview-hr.appspot.com/5552045/diff/1/codereview/views.py#newc...
codereview/views.py:3553: # TODO: Don't make the Repo field editable?  It could
just be hidden.
IMO we don't even need a hidden field since ther repo_id is already part of the
URL. In addition we could avoid the repo query to populate the drop down and it
would make the set_repo_choices() method obsolete.

[GvR, 2012-01-28 21:28:37]

Ok, how about now?

[Andi Albrecht, 2012-01-29 08:43:17]

LGTM

[gvrpython, 2012-01-29 19:43:10]

Thanks, submitted, but not in a hurry to deploy.

On Sun, Jan 29, 2012 at 12:43 AM, <albrecht.andi@googlemail.com> wrote:

> LGTM
>
>
http://py27.codereview-hr.**appspot.com/5552045/<http://py27.codereview-hr.ap...
>



-- 
--Guido van Rossum (python.org/~guido)

[techtonik, 2012-01-30 09:21:55]

Some comments not related to the reviewed patch (which seems to be a ok).

http://py27.codereview-hr.appspot.com/5552045/diff/1/codereview/views.py
File codereview/views.py (left):

http://py27.codereview-hr.appspot.com/5552045/diff/1/codereview/views.py#oldc...
codereview/views.py:249: class UploadPatchForm(forms.Form):
Why looking at what djangoforms is I've stumbled upon this article, which says
that it is impossible to use the Django forms with Google App Engine.

http://code.google.com/appengine/articles/djangoforms.html

But here I see that forms.Form is used even for Python 2.5. Should the article
be updated or I misunderstood something?

http://py27.codereview-hr.appspot.com/5552045/diff/1/codereview/views.py
File codereview/views.py (right):

http://py27.codereview-hr.appspot.com/5552045/diff/1/codereview/views.py#newc...
codereview/views.py:41: # TODO(guido): Don't import classes/functions directly.
I always wondered what is required to remove this TODO. Is now the proper time
to think about it again?

[gvrpython, 2012-01-30 18:25:13]

On Mon, Jan 30, 2012 at 1:21 AM,  <techtonik@gmail.com> wrote:
> Some comments not related to the reviewed patch (which seems to be a
> ok).
>
>
> http://py27.codereview-hr.appspot.com/5552045/diff/1/codereview/views.py
> File codereview/views.py (left):
>
>
http://py27.codereview-hr.appspot.com/5552045/diff/1/codereview/views.py#oldc...
> codereview/views.py:249: class UploadPatchForm(forms.Form):
> Why looking at what djangoforms is I've stumbled upon this article,
> which says that it is impossible to use the Django forms with Google App
> Engine.
>
> http://code.google.com/appengine/articles/djangoforms.html
>
> But here I see that forms.Form is used even for Python 2.5. Should the
> article be updated or I misunderstood something?

Exactly what sentence in that article are you referring to?

I think the misunderstanding may be that yes, you *can* use Django
forms with App Engine (always could), but no, you *cannot* use the
Django forms feature that automatically generates a form from a Django
database model class (and creates instances from the form response).

This latter restriction is because you cannot use Django model classes
with the App Engine datastore (at least not directly) and the App
Engine db model classes don't have enough metadata to work with the
Django forms library. But you can still create forms using the latter
-- you just can't tie them closely to datastore models.

The djangoforms module attempts to address this but it's not doing a
very good job, and in the mean time if you use more modern ways of
integrating Django with App Engine, such as djangoappengine or
django-nonrel, the problem is solved differently because those let you
use Django model classes with the App Engine datastore.

> http://py27.codereview-hr.appspot.com/5552045/diff/1/codereview/views.py
> File codereview/views.py (right):
>
>
http://py27.codereview-hr.appspot.com/5552045/diff/1/codereview/views.py#newc...
> codereview/views.py:41: # TODO(guido): Don't import classes/functions
> directly.
> I always wondered what is required to remove this TODO. Is now the
> proper time to think about it again?

We'd have to change these imports, which violate the style issue:

from cStringIO import StringIO
from xml.etree import ElementTree

from google.appengine.runtime import DeadlineExceededError

from django.http import HttpResponse, HttpResponseRedirect
from django.http import HttpResponseForbidden, HttpResponseNotFound
from django.http import HttpResponseBadRequest
from django.shortcuts import render_to_response

from django.template import RequestContext

from django.utils.safestring import mark_safe
from django.core.urlresolvers import reverse

from codereview.exceptions import FetchError  (also from models.py)

and adjust all the places where these functions and classes are used
to prefix them with the module name (and possibly fold long lines).

> http://py27.codereview-hr.appspot.com/5552045/

-- 
--Guido van Rossum (python.org/~guido)

[Andi Albrecht, 2012-01-31 07:16:35]

On Mon, Jan 30, 2012 at 7:24 PM, Guido van Rossum <guido@python.org> wrote:
> On Mon, Jan 30, 2012 at 1:21 AM,  <techtonik@gmail.com> wrote:
>> Some comments not related to the reviewed patch (which seems to be a
>> ok).
>>
>>
>> http://py27.codereview-hr.appspot.com/5552045/diff/1/codereview/views.py
>> File codereview/views.py (left):
>>
>>
http://py27.codereview-hr.appspot.com/5552045/diff/1/codereview/views.py#oldc...
>> codereview/views.py:249: class UploadPatchForm(forms.Form):
>> Why looking at what djangoforms is I've stumbled upon this article,
>> which says that it is impossible to use the Django forms with Google App
>> Engine.
>>
>> http://code.google.com/appengine/articles/djangoforms.html
>>
>> But here I see that forms.Form is used even for Python 2.5. Should the
>> article be updated or I misunderstood something?
>
> Exactly what sentence in that article are you referring to?
>
> I think the misunderstanding may be that yes, you *can* use Django
> forms with App Engine (always could), but no, you *cannot* use the
> Django forms feature that automatically generates a form from a Django
> database model class (and creates instances from the form response).
>
> This latter restriction is because you cannot use Django model classes
> with the App Engine datastore (at least not directly) and the App
> Engine db model classes don't have enough metadata to work with the
> Django forms library. But you can still create forms using the latter
> -- you just can't tie them closely to datastore models.
>
> The djangoforms module attempts to address this but it's not doing a
> very good job, and in the mean time if you use more modern ways of
> integrating Django with App Engine, such as djangoappengine or
> django-nonrel, the problem is solved differently because those let you
> use Django model classes with the App Engine datastore.
>
>> http://py27.codereview-hr.appspot.com/5552045/diff/1/codereview/views.py
>> File codereview/views.py (right):
>>
>>
http://py27.codereview-hr.appspot.com/5552045/diff/1/codereview/views.py#newc...
>> codereview/views.py:41: # TODO(guido): Don't import classes/functions
>> directly.
>> I always wondered what is required to remove this TODO. Is now the
>> proper time to think about it again?
>
> We'd have to change these imports, which violate the style issue:
>
> from cStringIO import StringIO
> from xml.etree import ElementTree
>
> from google.appengine.runtime import DeadlineExceededError
>
> from django.http import HttpResponse, HttpResponseRedirect
> from django.http import HttpResponseForbidden, HttpResponseNotFound
> from django.http import HttpResponseBadRequest
> from django.shortcuts import render_to_response
>
> from django.template import RequestContext
>
> from django.utils.safestring import mark_safe
> from django.core.urlresolvers import reverse
>
> from codereview.exceptions import FetchError  (also from models.py)
>
> and adjust all the places where these functions and classes are used
> to prefix them with the module name (and possibly fold long lines).

In general I support this style rule, but for Django projects it seems
very common to import classes and functions directly. See the tutorial
for example: https://docs.djangoproject.com/en/1.3/intro/tutorial03/

From my experience this makes the import part a bit chatty, but
especially the code for views a bit more readable. With module names
prepended Django tends to some kind of duplication, e.g.
mark_safe("some string") vs. safestring.mark_safe("somestring"),
reverse("some.view") vs. urlresolvers.reverse("some.view"),
HttpReponse("foo") vs. http.HttpResponse("foo").

But in the end it's matter of convention and both seem fine to me :)

--
Andi

>
>> http://py27.codereview-hr.appspot.com/5552045/
>
> --
> --Guido van Rossum (python.org/~guido)

[techtonik, 2012-01-31 07:54:32]

On 2012/01/30 18:25:13, gvrpython wrote:
> On Mon, Jan 30, 2012 at 1:21 AM,  <mailto:techtonik@gmail.com> wrote:
> > Why looking at what djangoforms is I've stumbled upon this article,
> > which says that it is impossible to use the Django forms with Google App
> > Engine.
> >
> > http://code.google.com/appengine/articles/djangoforms.html
> >
> > But here I see that forms.Form is used even for Python 2.5. Should the
> > article be updated or I misunderstood something?
> 
> Exactly what sentence in that article are you referring to?

The section "How do Django Forms Interact with the Datastore?" starts with:

   The Google App Engine model class, db.Model, is not the same as
   the model class used by Django. As a result, you cannot directly
   use the Django forms framework with Google App Engine.
 
> I think the misunderstanding may be that yes, you *can* use Django
> forms with App Engine (always could), but no, you *cannot* use the
> Django forms feature that automatically generates a form from a Django
> database model class (and creates instances from the form response).
> 
> This latter restriction is because you cannot use Django model classes
> with the App Engine datastore (at least not directly) and the App
> Engine db model classes don't have enough metadata to work with the
> Django forms library. But you can still create forms using the latter
> -- you just can't tie them closely to datastore models.
> 
> The djangoforms module attempts to address this but it's not doing a
> very good job, and in the mean time if you use more modern ways of
> integrating Django with App Engine, such as djangoappengine or
> django-nonrel, the problem is solved differently because those let you
> use Django model classes with the App Engine datastore.

That seems clear now that I know what `djangoforms` does, but I still have the
feeling that the article is confusing for those who try to understand the issue.
Thanks for the clarification.
 
> > http://py27.codereview-hr.appspot.com/5552045/diff/1/codereview/views.py
> > File codereview/views.py (right):
> >
> >
>
http://py27.codereview-hr.appspot.com/5552045/diff/1/codereview/views.py#newc...
> > codereview/views.py:41: # TODO(guido): Don't import classes/functions
> > directly.
> > I always wondered what is required to remove this TODO. Is now the
> > proper time to think about it again?
> 
> We'd have to change these imports, which violate the style issue:
> 
> from cStringIO import StringIO
> from xml.etree import ElementTree
> 
> from google.appengine.runtime import DeadlineExceededError
> 
> from django.http import HttpResponse, HttpResponseRedirect
> from django.http import HttpResponseForbidden, HttpResponseNotFound
> from django.http import HttpResponseBadRequest
> from django.shortcuts import render_to_response
> 
> from django.template import RequestContext
> 
> from django.utils.safestring import mark_safe
> from django.core.urlresolvers import reverse
> 
> from codereview.exceptions import FetchError  (also from models.py)
> 
> and adjust all the places where these functions and classes are used
> to prefix them with the module name (and possibly fold long lines).

That will decrease readability IMO. For readability it would be nice to separate
AppEngine stuff from Django to make it clear which API is used in particular
chunk of code, but I can't see a better solution than to move all long imports
into separate module like aeapi, aeext, which doesn't seem to have much sense
either.

So, should we just remove this TODO and leave everything as-is?

[GvR, 2012-01-31 18:34:34]

On 2012/01/31 07:54:32, techtonik wrote:
> So, should we just remove this TODO and leave everything as-is?

Sounds good.