Ok, so best would probably be, to include add the decorator definitions to the trunk, since they don't interfere with anything, maybe without the WikiPage adjustments in delete and save. And then the new db functionality, like my rename stuff, can start using it, so we can get experience.
Attached is a diff, that just contains the decorators.
So either apply this patch, or the one in the mail before with the changed WikiPage. Either way, some conclusion would be nice, so it's clear how to proceed.
> Jan Schukat wrote: >> If this proposal gets accepted, I would start converting all code to >> use the new scheme, as I have stated before.
> I wouldn't convert all database writes to the new scheme in one big > patch. Instead, I would rather introduce it with new code (IIRC, you > wanted to work on wiki page renames) and get familiar with it, maybe > refine the scheme somewhat with the experience gained, and only then, > transition old code progressively.
> Converting all writes at the same time sounds like a good way to > introduce subtle bugs all over the code. Instead, if the new > functionality is initially confined to a small section of code, the > potential damage should be confined as well.
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>)
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.
Jan Schukat wrote: > Ok, so best would probably be, to include add the decorator > definitions to the trunk, since they don't interfere with anything, > maybe without the WikiPage adjustments in delete and save. And then > the new db functionality, like my rename stuff, can start using it, so > we can get experience.
> Attached is a diff, that just contains the decorators.
This time the patch was fine for Thunderbird ;-)
Some additional comments I could have made for the previous patch as well: - from the rest of discussion, you should have seen that Remy and me agreed on a version without the "raise_exceptions" approach, so you shouldn't have reintroduced that, at least not without argumentation; same thing for the "return" part, as I already commented - you introduced two very similar decorators, with_transaction and with_db_transaction; the only benefit of the with_transaction one is to get a cursor from the db, which is something very trivial to do anyway (db.cursor()), so I don't think this warrants the extra complexity of having *2* decorators; with_db_transaction is enough and should be renamed with_transaction.
> So either apply this patch, or the one in the mail before with the > changed WikiPage. Either way, some conclusion would be nice, so it's > clear how to proceed.
Once we find the patch satisfying (i.e. all the feedback taken into account), we'll integrate it in trunk or perhaps first in a tmp branch, as we see fit.
(Remy wrote)
>> I wouldn't convert all database writes to the new scheme in one big >> patch.
Agreed.
>> Instead, I would rather introduce it with new code (IIRC, you >> wanted to work on wiki page renames) and get familiar with it, maybe >> refine the scheme somewhat with the experience gained, and only then, >> transition old code progressively.
Or, as there were also a bunch of #1106 specific issues to address first, do it for one module, exactly like you did in your transactions_py2.4.diff patch.
>> Converting all writes at the same time sounds like a good way to >> introduce subtle bugs all over the code. Instead, if the new >> functionality is initially confined to a small section of code, the >> potential damage should be confined as well.
Also agreed. But we might want to make all these changes on a branch, in order to get an overview of all the changes implied.
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
> 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))?
> 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.
Ok, a new patch, just one decorator, WikiPage implemented.
Also did some tests concerning the with-block variable scoping, got no problems this time, so I guess my problems earlier, while I was testing different approaches, were something entirely different.
Christian Boos wrote: > Hey, this one looks great!
I have got some minor nitpicks:
- Use WikiFormatting for docstrings.
- The "try: except: raise" in the case where db is not None is not necessary, just call fn(db).
- Sort imports alphabetically.
- Use "cursor" for cursors. That's the convention for the rest of Trac.
- There's still an "if(new_text)" in save().
> Remy, OK for an experimental branch, starting with this change?
A branch sounds a bit overkill for this, I would have opened a ticket and tracked progress with patches, and integrated the result into trunk. But I'm ok if you think it is worth the overhead.
Agreed with all nitpicks, and will prepare new patch on Sunday (I'm away now).
Except, what is the issue with the new_text variable? That's my way of moving validity checks out of the transaction, since they don't belong there and should be done before the transaction is started. I could make the checks multiple times though, first to rule out the error case, and then to distinguish between the two update types. Just makes the code longer and less clear IMHO.
Regards
Jan
Remy Blank <remy.bl...@pobox.com> schrieb am 16.10.2009 22:28:19:
> Christian Boos wrote: > > Hey, this one looks great!
> I have got some minor nitpicks:
> - Use WikiFormatting for docstrings.
> - The "try: except: raise" in the case where db is not None is not > necessary, just call fn(db).
> - Sort imports alphabetically.
> - Use "cursor" for cursors. That's the convention for the rest of Trac.
> - There's still an "if(new_text)" in save().
> > Remy, OK for an experimental branch, starting with this change?
> A branch sounds a bit overkill for this, I would have opened a ticket > and tracked progress with patches, and integrated the result into trunk. > But I'm ok if you think it is worth the overhead.
shoo...@email.de wrote: > Except, what is the issue with the new_text variable? That's > my way of moving validity checks out of the transaction, since > they don't belong there and should be done before the transaction > is started. I could make the checks multiple times though, first > to rule out the error case, and then to distinguish between the > two update types. Just makes the code longer and less clear IMHO.
I should have been more explicit, sorry. I was referring to:
if(new_text):
This looks so much like C code ;-) Use this instead:
> Christian Boos wrote: >> Hey, this one looks great!
> I have got some minor nitpicks:
> - Use WikiFormatting for docstrings.
> - The "try: except: raise" in the case where db is not None is not > necessary, just call fn(db).
> - Sort imports alphabetically.
> - Use "cursor" for cursors. That's the convention for the rest of > Trac.
> - There's still an "if(new_text)" in save().
>> Remy, OK for an experimental branch, starting with this change?
> A branch sounds a bit overkill for this, I would have opened a ticket > and tracked progress with patches, and integrated the result into > trunk. > But I'm ok if you think it is worth the overhead.
Jan Schukat wrote: > Ok, here the updated diff, with those little things taken care of.
Thanks!
I've now created http://trac.edgewall.org/ticket/8751 for tracking progresses on the sandbox/8751-with_transaction branch, which will be used to generalize this approach to the rest of Trac. Once the whole code base is converted and if in the meantime the approach is confirmed to be sound, we'll merge it back to trunk.
|
