1. Mail I'm using the OS X Mail.app now, don't think that is unable to handle mailing lists. I guess my mail provider is more at fault here and messes up MessageIDs. But that would be strange, since I used that same mail provider for apache-dev mailing lists, libstdc++-dev mailing lists and mozilla-dev mailing lists, and no one complained. Maybe I should dig out my gmail account, which I mainly use for spam catching now.
2. Me messing up Don't worry about how I might feel. When I'm wrong, I'm wrong, when I don't know something or missed something, then I don't know or missed something. No big deal. Only made very minor one line patch contributions to open source projects up to now though, so this is a little new to me, culture wise, as I mainly worked on internal projects for big corporations so far. Also, I lack experience in python, as you can tell, used it only for quick and dirty stuff up to now. Ok, enough excuses. 3. Actual issues - have read the python coding style now - only one decorator, that passes the db. Ok, wasn't sure about that anyway. - leave out raise_exceptions, always propagate - don't like the reference approach to pass values out, since it is sort of a hack, but it is structurally the same to what will be used later in the with statement, it's best I guess, will not be that common anyway - new patch with the adjustments and the WikiPage using the decorators will come shortly 4. Your test script - works fine, don't know what went wrong in my tests within the trac code - will look into that a little more Now I got to run some errands Jan On 15.10.2009, at 11:45, Christian Boos wrote: > > 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 -~----------~----~----~----~------~----~------~--~---
