Remove Python 2.4 compatibility

[Remy Blank]

The goal of this task is to remove Python 2.4 compatibility code, and to start using Python 2.5-only features. See ​what's new in Python 2.5. This includes:

• Use context managers and the with statement for: database transactions, handling locks, handling files.
• Use ... if ... else ... expressions instead of the more error-prone ... and ... or ....
• Use try: except: finally: instead of nested try: finally: and try: except:.
• Use except Exception: instead of except:.
• Remove our own implementations for functionality that was introduced in Python 2.5.
• Use generator expressions instead of list comprehensions where adequate.
• Use tuple arguments to str.startswith() and str.endswith().
• Use relative imports for parent, sibling and child modules?
• Remove workarounds for 2.3 and 2.4.

Please add new items to this list as they pop up.

[Christian Boos]

• Use ​ context managers and the `with` statement

The change concerning the context managers and the with statement mostly concerns the refactor transaction handling topic (#8751), as discussed in the ​googlegroups:trac-dev:4efe0b99b4e78d23 mail and follow-ups.

But we could also use it for other things, like holding locks.

Slightly related, a few places could also benefit from the unification of ​ try/except/finally.

[Christian Boos]

• Use ... if ... else ... expressions instead of the more error-prone ... and ... or ....

Btw, Genshi supports this at least since the merge of the AST branch (see ​log:trunk/scripts/ast_generator.py) nearly 2 years ago, so we can also use that in templates (same thing for generator expressions).

[Remy Blank]

Copying items to the description, to get a nice overview. Keep 'em coming!

We should probably also start using except Exception: instead of except:.

What about using relative imports for parent, sibling and child modules (e.g. use from .api import ...)? I wouldn't want to use relative imports for cross-package imports though.

[Christian Boos]

The change concerning the context managers and the with statement mostly concerns the refactor transaction handling topic (#8751), as discussed in the ​googlegroups:trac-dev:4efe0b99b4e78d23 mail and follow-ups.

I'd like to use with env.db_transaction as db (TracDev/ApiChanges/0.13@3#DatabaseAPIChanges).

See 9536-db-context-manager-r10102.patch​.

Example usage:

trac/versioncontrol/api.py

@@ -205 +205 @@
         if type_ and type_ not in rm.get_supported_types():
             raise TracError(_("The repository type '%(type)s' is not "
                               "supported", type=type_))
-        @self.env.with_transaction()
-        def do_add(db):
+        with self.env.db_transaction as db:
             id = rm.get_repository_id(reponame)
             cursor = db.cursor()
             cursor.executemany("""

As you can see in the patch, I'd like to have the context manager to be simpler to use than the @with_transaction decorator of 0.12, in particular we assume no one will obtain a database connection by other means so no need to keep a db parameter. I'll do more clean-ups in that direction later (in particular to handle the issue discussed in #9470).

[Christian Boos]

db: introduce context managers for accessing Connection instances.

[Remy Blank]

Replying to cboos:

See 9536-db-context-manager-r10102.patch​.

A few comments:

• Why set self._env to None after getting the connection? Wouldn't it be more intuitive to set a _db = None at class scope, and check for that in __exit__() to decide if a commit or rollback is needed?
• Does it make sense to use "private" attributes like _env and _db, when the context manager won't ever be accessible to the users anyway? I would prefer dropping the underscores.
• I like the idea of preventing commits and rollbacks in query connections, but could we add that functionality to ConnectionWrapper instead? (I haven't got time to look now, so correct me if I'm wrong.) (Yes, I'm wrong, we probably need another wrapper.)

[Christian Boos]

Replying to rblank:

Replying to cboos:

See 9536-db-context-manager-r10102.patch​.

A few comments:

• Why set self._env to None after getting the connection? Wouldn't it be more intuitive to set a _db = None at class scope, and check for that in __exit__() to decide if a commit or rollback is needed?

The idea was that the _env was "consumed" after being used. Your idea is slightly better, though.

• Does it make sense to use "private" attributes like _env and _db, when the context manager won't ever be accessible to the users? I would prefer dropping the underscores.

Well, it's really a private, but as it's highly unlikely that someone would write db = env.db_transaction.__enter__().db, I concede we could drop the _.

• I like the idea of preventing commits and rollbacks in query connections, but could we add that functionality to ConnectionWrapper instead? (I haven't got time to look now, so correct me if I'm wrong.)

Well, I first thought about subclassing, but as the main method __getattr__ would have had to be reimplemented anyway, it was not worth the import… However we could add a flag readonly, and that would be shorter.

Refined patch in 9536-db-context-manager-r10102.2.patch​.

Now a bit of clean-up remains to be done. We currently have the following clumsy sequence:

1. Environment.db_transaction →
2. trac.db.api.TransactionContextManager →
3. Environment.get_db_cnx() →
4. trac.db.api.get_read_db() →
5. _transaction_local.db or DatabaseManager(env).get_connection()

We have 1. for the same reason we had Environment.with_transaction so that we don't need to import trac.db.api from everywhere. For 2. it's fine to have the context managers themselves in trac.db.api. Now, 3. is problematic: at least we should rename it to _get_db_cnx to discourage its use (would that be enough? people would also do the same rename and curse us ;-) ). The interest of having to go through the Environment here as opposed to inline directly DatabaseManager(env).get_connection() is that the tests (and more precisely the EnvironmentStub) do some hacking at this level in order to reuse one single DatabaseManager instance across all test environments, especially useful when dealing with for PostgreSQL and MySQL backend.

I'd really prefer to inline DatabaseManager(env).get_connection() in the context managers and fine a way to make the tests work nevertheless.

[Christian Boos]

Updated patch, reuse ConnectionWrapper

[Remy Blank]

Replying to cboos:

Well, I first thought about subclassing, but as the main method __getattr__ would have had to be reimplemented anyway, it was not worth the import… However we could add a flag readonly, and that would be shorter.

What still bothers me is that for nested queries, or queries nested in a transaction, this wraps the connection repeatedly. We probably won't have more than 3-4 levels of nesting, but it's still… unaesthetic. We could check if the connection is already wrapped as read-only, and only wrap if it isn't. That would only re-wrap the first query level in a transaction, and we probably can't avoid that.

Now a bit of clean-up remains to be done. We currently have the following clumsy sequence:

I agree this should be simplified.

[Christian Boos]

Replying to rblank:

Replying to cboos:

Well, I first thought about subclassing, but as the main method __getattr__ would have had to be reimplemented anyway, it was not worth the import… However we could add a flag readonly, and that would be shorter.

What still bothers me is that for nested queries, or queries nested in a transaction, this wraps the connection repeatedly. … We could check if the connection is already wrapped as read-only, and only wrap if it isn't.

Sure.

Now a bit of clean-up remains to be done. We currently have the following clumsy sequence:

I agree this should be simplified.

Ok, I think I found a way which is not that bad (I just hope I won't get shot by the anti-monkey-patching patrol ;-) ).

Please have a look at 9536-db-context-manager-r10102.3.patch​.

For the follow-up clean-ups, I think we should keep the db or cnx parameters, but document the fact they're not used anymore and are going to be removed in 0.14.

And while we're at refactoring the db queries, what about adding an "execute" convenience method on our ConnectionWrapper, so that instead of:

        with self.env.db_query as db:
            cursor = db.cursor()
            cursor.execute("""
                SELECT type, id, filename, time, description, author
                FROM attachment
                WHERE time > %s AND time < %s AND type = %s
                """, (to_utimestamp(start), to_utimestamp(stop), realm))
            for realm, id, filename, ts, description, author in cursor:
                time = from_utimestamp(ts)
                ...

we could just write:

        with self.env.db_query as db:
            for realm, id, filename, ts, description, author in db.execute("""
                SELECT type, id, filename, time, description, author
                FROM attachment
                WHERE time > %s AND time < %s AND type = %s
                """, (to_utimestamp(start), to_utimestamp(stop), realm)):
                time = from_utimestamp(ts)
                ...

[Christian Boos]

only wrap once with a readonly ConnectionWrapper, and remove trac.env / trac.db.api round trips for obtaining a connection

[Christian Boos]

I've been busy those last days with some heavy refactoring of the db api, following up the changes above. I'm running a last complete test with the postgres, mysql and sqlite (file and in-memory) to be really sure, but it should all work now. …

Yes, testing with the regular SQLite backend and not only the in-memory one is now possible, a long story (ticket:8169#comment:22) which comes to an end ;-)

Would you be OK with me committing directly the changes on trunk?

The changes come in two sequences, first the trac/db and trac/tests changes, then updates of every other part of the code to use the new style (more than 2600+ and 2400- as I also did lots of other smaller style clean-ups along the way).

For now, the queries are using the with self.env.db_query as db: style as in the first example of comment:8, but I'd actually like to go with one of the following:

with env.db_query as db:     
    for a, b, c in db.select("""
            a, b, c FROM test
            """):
       ...

if we need the db instance (e.g. for db.quote or db.cast needed), or if we don't need it, directly:

for a, b, c in env.db_query.select("""
        a, b, c FROM test
        """):
    ...

The advantage of using select("a, b, c") over execute("SELECT a, b, c") is that we can't misuse the "read-only" Connection.

[Remy Blank]

Replying to cboos:

Would you be OK with me committing directly the changes on trunk?

Sure, we can do any fine tuning on trunk if necessary.

For now, the queries are using the with self.env.db_query as db: style as in the first example of comment:8, but I'd actually like to go with one of the following:

with env.db_query as db:     
    for a, b, c in db.select("""
            a, b, c FROM test
            """):
       ...

What's the use of a context manager whose __exit__() is a no-op? Couldn't env.db_query just return the wrapped read-only connection?

The advantage of using select("a, b, c") over execute("SELECT a, b, c") is that we can't misuse the "read-only" Connection.

Nice idea. Using a select() generator method means the cursor will be kept in the generator frame. Hopefully this won't keep open cursors around, e.g. in the case where the iterator isn't consumed completely. Also, this will instantiate a new cursor for every SELECT, but I assume this isn't too expensive an operation.

[Christian Boos]

Replying to rblank:

Replying to cboos:

Would you be OK with me committing directly the changes on trunk?

Sure, we can do any fine tuning on trunk if necessary.

Great, thanks!

The complete set of tests did work fine. However in some situations I still have to fight against a strange:

psycopg2.ProgrammingError: permission denied for relation system

I hope it won't take me too long…

For now, the queries are using the with self.env.db_query as db: style as in the first example of comment:8, but I'd actually like to go with one of the following:

with env.db_query as db:     
    for a, b, c in db.select("""
            a, b, c FROM test
            """):
       ...

What's the use of a context manager whose __exit__() is a no-op? Couldn't env.db_query just return the wrapped read-only connection?

I think doing this change at the same occasion as the with env.db_transaction one is good for a) symmetry reasons with the write operation (only one "idiom" to use), b) the visual change which makes apparent the "duration" of the connection, and finally c) it's likely that it won't stay a no-op…

Even though I didn't implement that yet, I'd like to find ways to enforce that the db is not used longer than it should. Closing the cursors opened during that time could be a possible exit operation. Also, do you remember that discussion about relying on the gc for returning the connections to the pool? Doing an explicit return from the outer context's __exit__ would be a possible solution.

The advantage of using select("a, b, c") over execute("SELECT a, b, c") is that we can't misuse the "read-only" Connection.

Nice idea. Using a select() generator method means the cursor will be kept in the generator frame. Hopefully this won't keep open cursors around, e.g. in the case where the iterator isn't consumed completely.

Well, doing this would maintain the cursor alive, and therefore the connection for pysqlite. The idea of using select(...) is on the contrary to consume the cursor entirely in order to use the connection the shortest time possible. That's what we do anyway for sqlite, with the EagerCursor, which btw would then become useless. If in the future there are places where this could be a problem, we would need to use LIMIT.

Also, this will instantiate a new cursor for every SELECT, but I assume this isn't too expensive an operation.

That shouldn't be a problem, no. What is problematic is that we have really too many different SQL queries, it would be good to cut that down a bit so that we could benefit more from the statement cache in PySqlite ;-)

[Remy Blank]

I have cleaned up trac.util.compat and fixed all occurrences where it is imported in [10128].

[Remy Blank]

Removed some compatibility code in [10129].

[Christian Boos]

Great! On my side, I pushed the ideas in comment:11 further and now I'm close to have it all working in a way I like. However the changes are quite big, so I'll push those in a clone first, so that we can discuss them.

[Christian Boos]

Please have a look at the branch db-context-managers. I did some extensive review and testing, but as the changes are also quite extensive, it's possible I've missed one thing or two…

I'm especially interested in comments on the first patch [e74279c9ae85/cboos], which introduces the changes at the trac.db level. Note that this change one alone doesn't pass the tests, the next changeset is needed for that, as it fixes the test machinery. The follow-up changes are mainly for applying the new style to the rest of the code, module by module.

Ah yes, I didn't add the from __future__ import with_statement for 2.5 yet, so if you'd like to test, use 2.6 or 2.7.

[Christian Boos]

With r10189, the branch has been merged to trunk.

Now the next cosmetic fix I have in mind is improving the imports, by using multiline imports and relative imports…

[Christian Boos]

On the files touched by new changes, we could also do some preliminary clean-ups, like I did for example in r10239.

[Christian Boos]

I replaced a bunch of finally statements with their context manager counterpart in attachment:9536-cleanup-finally-r10401.diff​.

All tests pass, even with Python 2.5, but an extra review wouldn't hurt.

[Christian Boos]

use with for automatically closing files and releasing locks.

[Christian Boos]

9536-cleanup-finally-r10401.diff​ applied in r10402.

[Remy Blank]

First part of the conversion from and - or to if - else.

[Remy Blank]

I have started converting the most obvious uses of ... and ... or ... into ... if ... else ... (current state above). There are quite a lot of them, so before I do them all, I'd just like to get a quick "ok" or "ko" for the change. Personally, I find the if - else construct much clearer.

[Christian Boos]

I've reviewed the patch, all changes look correct (at least I couldn't spot an error ;-) and the few semantic changes seem for the better). So yes, +1 and if you'd like a review on the next batch, don't hesitate to post it here as a follow-up patch.

[Remy Blank]

First batch of changes applied in [10580].

[Remy Blank]

Second (and final) "and or → if else" batch.

[Remy Blank]

9536-and-or-if-else-2-r10590.patch​ is the second and final batch of conversions to "if else". A review would be appreciated.

[Christian Boos]

Done (a few smaller batches would have hurt less ;-) ).

All good things, the only suggestion I'm able to make is the following:

trac/ticket/templates/report_view.html

@@ -86 @@
-${('desc', 'asc')[header.asc] if header.asc is not None else None}
+${'asc' if header.asc else 'desc' if header.asc is not None else None}

+ all tests passed with 2.5.4.

I was a bit surprised with the from __future__ import ... working even with a docstring before it, don't know why I thought this had to be the very first statement. Being the very first import is enough, apparently.

Also, in a few places in the templates I've seen in the patch, we could make use of the classes() utility, and even add a styles() one. I can do that as a follow-up.

[Remy Blank]

Replying to cboos:

Done (a few smaller batches would have hurt less ;-) ).

Heh, so you can imagine how much it has hurt to create the patches in the first place ;)

All good things, the only suggestion I'm able to make is the following:

Yes, I messed up that one.

I was a bit surprised with the from __future__ import ... working even with a docstring before it, don't know why I thought this had to be the very first statement. Being the very first import is enough, apparently.

Being the first import is the criterion. I got the hint from pylint that the docstring was a no-op (i.e. not interpreted as a docstring) if it is after the import.

Thanks for the review!

[Remy Blank]

Second batch applied in [10594].

[Remy Blank]

Cleaned-up exception handling in [10595].

[Remy Blank]

Multiple calls to .startswith() and .endswith() fixed in [10596].

[Remy Blank]

Changed a few easy list comprehensions into generator expressions in [10597]. I haven't found a good regexp to find other possible candidates, so I'll just convert them as I find them.

I'm through with the cleanup. Keeping open for comment:25.

[Christian Boos]

On the topic of language related clean-ups, I've seen the following this morning on mercurial-dev:

hasattr() in Python swallows all exceptions rather than only AttributeError, making it unsafe for general use. getattr(a, b, None) is a mostly-compatible replacement (it fails to distinguish missing attributes and None-value attributes) that we can use instead.

We have about 50 occurrences of hasattr, do you think it's worth fixing?

[Remy Blank]

Replying to cboos:

We have about 50 occurrences of hasattr, do you think it's worth fixing?

If we do, we should decide on a case-by-case basis what the replacement should be. Sometimes the hasattr() call is even redundant, as it is followed by a getattr(), sometimes it can be replaced by a try: except AttributeError: block, sometimes a default value could be set at the class level.

And sometimes the hasattr() call is needed. In that case, I would replace the call with getattr(a, b, noattr) is not noattr where noattr is a marker object (which could simply be defined as noattr = object()).

Should we do the change? The buggy behavior of swallowing all exceptions is only an issue when hasattr() is called on an object with a custom __getattr__() method. So we could probably leave most calls alone, and just fix the ones that are really affected.

[Christian Boos]

Replying to rblank:

Should we do the change? The buggy behavior of swallowing all exceptions is only an issue when hasattr() is called on an object with a custom __getattr__() method. So we could probably leave most calls alone, and just fix the ones that are really affected.

Ok, we have a couple __getattr__ (10), so maybe only a handful of those 50 calls (if any) would need to be replaced.

[Christian Boos]

So I think we're done with this one.

The hasattr/getattr clean-up can be done on a case-by-case basis.

[Ryan J Ollos]

It looks like there's a unbound variable in [10180]. It didn't seem worthwhile to open a ticket for such a small issue, so I'll just post the change here for confirmation before committing:

trac/test.py

@@ -285 +285 @@
 
         init_global = False
         if self.global_databasemanager:
-            self.components[DatabaseManager] = global_databasemanager
+            self.components[DatabaseManager] = self.global_databasemanager
         else:
             self.config.set('trac', 'database', self.dburi)
             self.global_databasemanager = DatabaseManager(self)

The patch is from Olemis Lang.

[Ryan J Ollos]

Change from comment:35 committed to 1.0-stable in [12285] and merged to trunk in [12286].

[Ryan J Ollos]

[10180#file3] contained an unused import of ConnectionWrapper that looks to be left over from 9536-db-context-manager-r10102.3.patch​. Removed on 1.0-stable in [13569], merged in [13570].

[Ryan J Ollos]

One other question about [10180]. Is there a reason to keep the following line?: tags/trac-1.0.2/trac/test.py@:358#L328​

[Jun Omae]

I think no reason and we could remove the line, db = None # as we might .... I just confirmed that unit tests with all database backends pass after removing the line on my environment.

[Ryan J Ollos]

Thanks for review. Committed to 1.0-stable in [13575] and merged to trunk in [13576].

[Ryan J Ollos]

Another question about [10180] that I've been wondering for some time: what is the purpose of global_databasemanager? I could not find a case in which this code branch is executed: trunk/trac/test.py@14779:387#L381​.

I also couldn't find a reason that False needs to be returned by destroy_db: trunk/trac/test.py@14779:447#L437​.

I've prepared some changes in log:rjollos.git:t9536_refactoring, to consider for Trac 1.2.

[Christian Boos]

Replying to Ryan J Ollos:

Another question about [10180] that I've been wondering for some time: what is the purpose of global_databasemanager? I could not find a case in which this code branch is executed: trunk/trac/test.py@14779:387#L381​.

You're right. In retrospect, I probably failed to clean-up properly that change.

I also couldn't find a reason that False needs to be returned by destroy_db: trunk/trac/test.py@14779:447#L437​.

ditto.

I've prepared some changes in log:rjollos.git:t9536_refactoring, to consider for Trac 1.2.

However there was a reason to avoid relying on the component manager in order to find the database manager… although I can't remember the specifics. But if everything still passes and there's no adverse effect (e.g. on speed), then I'm all for the cleanup ;-)

The only reservation I have is to add yet another clean-up on 1.2dev, I think "we" really should release it at some point on move on with the development on 1.3dev.

Or just make a stabilization branch 1.2-stable and a 1.2rc1 out of it, if you really would not like to wait doing things on trunk and think that there's still quite some work to do before 1.2.

[even more OT] I'll be away till July, but when I'm back I'll have time to focus on Jinja2 migration again, so it would be really great if trunk would be on 1.3dev at that time!

[Ryan J Ollos]

Replying to Christian Boos:

The only reservation I have is to add yet another clean-up on 1.2dev, I think "we" really should release it at some point on move on with the development on 1.3dev.

Or just make a stabilization branch 1.2-stable and a 1.2rc1 out of it, if you really would not like to wait doing things on trunk and think that there's still quite some work to do before 1.2.

Yeah, we should release Trac 1.2. There isn't much work to do. I've just been going through some notes I made over the past two years, which is leading to the cleanup. I'd like to finish the integration of TracMigrate, but if that can't be done soon I'll wait until 1.3.1.

I'll make 1.2-stable this week, and set the trunk to 1.3.1dev so that I can start pushing changes to #11901 while we go through the release candidates for 1.2.

[Ryan J Ollos]

comment:41 changes committed in [14839].