|
Graham Dumpleton
grahamd at dscpl.com.au
Fri Oct 27 02:42:38 EDT 2006
Mike Looijmans wrote ..
> I for one like the changes - it's much cleaner this way. There's only
> one list to deal with now, and that list handles all access..
>
> Just one minor optimization, in __len__ i think you can replace:
> return len(self.list.table().keys())
> with
> return len(self.list.table())
> since that prevents the keylist from being generated.
Done.
> We might solve all the "dictionary" questions if we provide a
> "getdict()" method (which would return a copy of list.table()).
I am wary about making access to the dictionary part of any public API
as then we end up in the same sort of situation we have with list where
people modify it directly and then perhaps wander why there changes
are being reflected in the list. Better to hide the dictionary and mediate
all access for a public API we define in FieldStorage.
> Another,
> idea is, could we support the following:
>
> form = FieldStorage(...)
> myfrm = {}
> myfrm.update(form)
That works now. possibly because of the items() method.
> This would give those who really want that dictionary a real dictionary
> to work with, if they don't need the fancy FieldStorage features.
The code provides the basic dictionary type operations already.
As brought up in prior email about expected behaviour, we could
sensibly provide __delitem__, but what __setitem__ does is a bit
open for question. Trac already sets some precedent by saying it is
additive which isn't dictionary like at all. But then __getitem__
doesn't work like a normal dictionary either. Thus, question is
what the consensus is about what __setitem__should do.
Graham
> Mike Looijmans
> Philips Natlab / Topic Automation
>
>
> Graham Dumpleton wrote:
> > Mike Looijmans wrote ..
> >
> >>Being the one who moved all that code around, I'm probably the one to
> >>explain why.
> >>
> >>In the original version, there was a 'bad' way of detecting whether
> >>objects were files or not. If one used the callback mechanism, and
> >>provided a file-like object, this object would not be recognised as a
> >>file by that code, and that either resulted in the whole file being read
> >>into memory as a string or in some obscure exception being thrown.
> >
> >
> > Okay, so more than just performance issues with field access.
> >
> > Anyway, I have committed some changes into repository now which
> > fixes the issue with add_field() but also does things in such a way that
> > code such as Trac which creates Field instances direct and adds them
> > to the the list of form fields will still work. It does this by using
> a
> > derived list class instance which invalidates the key lookup table on
> > changes to the list so that the lookup table is rebuilt the next time
> > it is required. Doing this wasn't as bad as I first thought it would
> be
> > as hadn't gone to the extent of properly understanding the code before.
> > Also include a semi hack in Field class constructor to detect old style
> > use where code expects more than just the name of the field as
> > argument and do appropriate initialisation as appropriate.
> >
> > I'd add a comment to JIRA, but it seems to be playing up at the moment
> > or at least firewall where I work is screwing up as it tends to do a
> bit
> > when JIRA is accessed using https from here.
> >
> > In all, what the changes allow is for the following to work:
> >
> > from mod_python import util
> > from StringIO import StringIO
> >
> > def index(req):
> > req.content_type = 'text/plain'
> >
> > req.write('%s\n\n' % req.form.keys())
> >
> > for key in req.form.keys():
> > req.write('%s %s\n' % (key, req.form.getlist(key)))
> >
> > req.write('\n')
> >
> > req.form.add_field('e', 'f')
> >
> > req.form.list.append(util.Field('g', StringIO('h'),
> > "text/plain", {}, None, {}))
> > req.form.list.extend([util.Field('i', StringIO('j'),
> > "text/plain", {}, None, {})])
> > req.form.list += [util.Field('k', StringIO('l'),
> > "text/plain", {}, None, {})]
> >
> > req.write('%s\n\n' % req.form.keys())
> >
> > for key in req.form.keys():
> > req.write('%s %s\n' % (key, req.form.getlist(key)))
> >
> > req.write('\n')
> >
> > return None
> >
> > If this is accessed with:
> >
> > http://localhost:8002/~grahamd/form/?a=b&c=d&e=f
> >
> > I get:
> >
> > ['a', 'c', 'e']
> >
> > a [Field('a', 'b')]
> > c [Field('c', 'd')]
> > e [Field('e', 'f')]
> >
> > ['a', 'c', 'e', 'g', 'i', 'k']
> >
> > a [Field('a', 'b')]
> > c [Field('c', 'd')]
> > e [Field('e', 'f'), Field('e', 'f')]
> > g [Field('g', 'h')]
> > i [Field('i', 'j')]
> > k [Field('k', 'l')]
> >
> > As the req.form.keys() method works off the lookup table, you can see
> > how updates direct to list work okay, meaning stuff like Trac should
> > work.
> >
> > You will also note that the 'dictionary' attribute has gone and is hidden
> > elsewhere. Hopefully people will not try and directly access the
> > equivalent, but some stern notes in the documentation saying only
> > to use documented interface might help. :-)
> >
> > If you can test the updates and especially if someone can test it with
> > older versions of Trac that would be great.
> >
> > Graham
> >
> >
> >>Graham Dumpleton wrote:
> >>
> >>>I'd probably hold off trying to work out any patches yourself. The more
> >>>I look at FieldStorage the less I understand why some of the changes
> >>>were made. I'm not talking about how a dictionary based index was
> >>>added, but more how some of the code was shifted around when it
> >>>probably didn't need to, with the result being that the code then became
> >>>incompatible with versions of Trac which were out at the time. It would
> >>>not have been that hard to maintain compatibility with Trac and still
> >>
> >>get
> >>
> >>>the desired gains they were after. I can see myself therefore doing
> a
> >>
> >>bit
> >>
> >>>or reorganisation of the code to fix the identified issue, but also
> >>>reinstate
> >>>Trac compatibility.
> >
> >
>
>
> --
> Mike Looijmans
> Philips Natlab / Topic Automation
>
> _______________________________________________
> Mod_python mailing list
> Mod_python at modpython.org
> http://mailman.modpython.org/mailman/listinfo/mod_python
|