Hello Jan, Thanks for your continued efforts, we're getting there ... but not yet.
First, some general remarks. Apparently your mail program and my Thunderbird are not friendly to each other ;-) The first patch (transactions_py2.5.diff) was completely garbled. From the web interface however, the patch can be obtained correctly (http://groups.google.com/group/trac-dev/msg/dfbfd93adaabfb06?hl=en). So, in doubt, let's blame Thunderbird here, but I would be interested to know how it worked for others. The second patch (transactions_py2.4.diff) contained the changes *twice*. You must have done svn diff >> .diff at some point... Ok, those were uninteresting comments about the form of the patches, sorry but this is just getting in the way of being able to review comfortably the changes. Now, slightly more on topic, you should really try to conform to the coding style adopted by the Trac project, as those are prerequisites for any changes (remember that I already made such comments in #1106 - (1)). In particular, we try to conform to the standard Python coding style (PEP 8 (2)), and there are constructs in your changes that are quite at odds with those guidelines (more than one statement per line, if(test), ...). Jan Schukat wrote: > Hello again, > > I have installed a python2.4 again, and created two implementations of > the transaction scheme, one for 2.4, which uses decorators, and one > for 2.5, which uses with statements. In both cases I converted the > WikiPage class to use the scheme, and both pass the complete unit test > and functional test suits in the python versions they are supposed to > work in. > Good, that's indeed *the* recommended way to validate the changes. Writing additional tests for newly introduced features is also useful. > Both are very very similar in their use, but I do think now, it would > be unwise to have both implementations present at the same time in the > code. Whether both implementations are present or not, it wold need > need cleanup, once 2.4 support is dropped, and in the case of just > having to replace the decorators with with-statements and the context > managers, without all that version check code, this could be done with > a quite simple sed script ... > Right, we only need the decorator based approach for now. Doing the context manager based approach during the experimental stage is nevertheless important so that we can be confident we're on the right track and that the migration will be painless. > What I did notice though is, that you can't get variables out of the > with block. The overwriting of the new_name variable in the Trac 0.13 > example by Christian below isn't visible outside the with block. At > least that is what the unit tests told me when I tried it. Maybe I'm > not aware of something here. > Well, it should work. What do you see when you run my test program (the one from (3))? $ /C/Dev/Python254/python with_transaction.py ... == (fail=False, do_rename=<function do_rename_with at 0x027E6970>) -- testing automatic transaction >> Environment.get_db_cnx >> execute something successfully... >> commit transaction > it worked: renamed -- testing explicit transaction >> Environment.get_db_cnx >> execute something successfully... > it worked: renamed >> execute something successfully... > it worked: renamed >> commit transaction ... The "it worked: renamed" part shows that the assignment in the with block modified the reference from the containing block: # Trac 0.13 def do_rename_with(env, db=None): new_name = None with transaction(env, db) as db: db.execute() new_name = 'renamed' # modifies `new_name` above if new_name: print '> it worked:', new_name return True This should work, therefore we should also use the modification "by reference" suggested by Remy for the decorator approach, modifying the single element of an array, rather than my clumsy way of returning a value via the function name ;-) Btw, please take all of this as /constructive/ criticisms, I wouldn't bother making them if I wasn't interested in seeing your contributions integrated in the end ;-) Now heading to your next patch... -- Christian > If this proposal gets accepted, I would start converting all code to > use the new scheme, as I have stated before. > If there are still objections, I would like to hear them. > (1) - http://trac.edgewall.org/ticket/1106#comment:110 (2) - http://www.python.org/dev/peps/pep-0008/ (3) - http://groups.google.com/group/trac-dev/msg/7ec20d1f259aa046 --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Trac Development" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [email protected] For more options, visit this group at http://groups.google.com/group/trac-dev?hl=en -~----------~----~----~----~------~----~------~--~---
