Gregory (Grisha) Trubetskoy wrote:

Having looked at the FieldStorage code, I'm guessing the idea was that you parse fields as they come in and append them to a list. This preserves the original order of fields, in case it is needed.

I assumed that as well, but I'm not sure getting the fields in a particular order is the most common use case - not for me anyway. Plus, I'm not suggesting getting rid of access to FieldStorage.list. For the dict-like such as fs.keys(), I don't think people should expect keys in any particular order. If they really want that feature then they can subclass write their own ordered dict class.

I'm not sure that maintaining a dictionary alongside the list is the right thing to do. It might be, but there are some difficult questions to answer -e.g. how costly is a sequential search, and is the code complexity (and fieldstorage code is no picnic to read as it is) worth the speedup?

The current code is a litte hairy - it helps to drink a cup of strong coffee before reading it. We can always hide the complexity in a separate add_field method. As to the performance tradeoffs, I guess we benchmark and see? I love doing benchmarks. The 2 things that bring joy to my heart are benchmarks and unit tests. And good documentation. The *3* things that bring joy to my heart are... well you get the idea.

Also while it would speed up retrieval, it will slow down the "write" operation - when a field is added to fieldstorage you now need to append it to the list, AND check whether it exists in the dictionary, then add it there as well.

How often do developers access form fields via __getitem__? I noticed the publisher does not use it - it iterates the list, so nothing would be gained there.

For myself, I use it (almost?) exclusively as a dict. As for the use in publisher, maybe that implementation needs to be examined as well. ;)

Also, something else to consider - is there a simple programatic solution that could be documented, e.g. something like

my_fs = util.FieldStorage(req)

dict_fs = {}

[have no idea whether this will work :-)]

Nope. If you have multiple fields with the same name you'll lose all but the last field. (eg. The checkbox example example on the mod_python list that got me started on this in the first place).

and voila - you've got a dictionary based fieldstorage?

Except that FieldStorage is already supposed to be dict-like so why would I want to duplicate the effort in my code? For example 7 out of 10 the fs methods are there to support dict-like behaviour and the other 3 are initialization helpers which will never be called by user code anyway. To me, that makes it a dictionary. I'm not talking about adding new functionality to FieldStorage, just examining the current implementation wrt to performance.

Anyway, just a few cents from me.

I don't want you to think I'm hung up on this issue. It just seems to me that the goal of mod_python moving forward should be stability, speed, efficiency while keeping feature creep to a minumum. I think it's worthwhile to examine existing code as we go along to see if we are meeting these goals. We still need to have *some* code to chew on every now and then after all. :)


Reply via email to