Patches for 1.3.2+ compatibility

[Jonathan Laufersweiler]

I've been running 1.3.2 for a few months and updated SimpleMultiProject to be compatible. I've been using it on our department's Trac for several weeks now with no problems.

I did a brief spin at 1.3.3dev, and the logic seemed fine, but it needed all the new jinja2 templates for roadmap etc reverted back to their genshi versions to work, so I haven't done any further testing there yet. The dbapi and environment upgrade changes should provide a foundation for any genshi -> jinja2 work going forward though.

Most changes involved things like replacing:

@with_transaction(self.env)
def execute_sql_statement(db):
    cursor = db.cursor()

with

def execute_sql_statement():
    with self.env.db_transaction as db:
        cursor = db.cursor()

...or adding None to template returns to force genshi rather than jinja2 rendering for the existing templates.

Attached is one .patch file per source file changes and a comprehensive .diff file.

[Ryan J Ollos]

Only the diff of all files is needed. Regarding coding style for the database API, see: ​TracDev/DatabaseApi#Trac1.0API.

[Jonathan Laufersweiler]

Was the link to the db API just for reference within the ticket, or have I failed to follow it? I (thought I) was using that as a guide.

[Ryan J Ollos]

Example:

with self.env.db_query as db:
    cursor = db.cursor()
    cursor.execute('SELECT * FROM milestone_version')
    for row in cursor:
        data.update({row[0]: row[1]})

->

for row in self.env.db_query("""
        SELECT * FROM milestone_version
        """):
    data.update({row[0]: row[1]})

That could be simplified even further:

data = [dict(milestone, version) for milestone, version in self.env.db_query("""
        SELECT milestone, version from milestone_version
        """)]

Untested, so I may have some obvious typos in the expressions.

[Jonathan Laufersweiler]

I can see where that approach would be more performant, and appreciate the clever use of for as an implicit context manager for the db than explicitly declaring one via with as the api guide demonstrates. I don't know that I'd call the list comprehension approach "simpler" per se, but I can see how pruning more context switches and method calls might be even faster.

I'd be curious though as to whether the lessened interpreter overhead is significant compared to the time for the db query. Database specifics very widely of course, buy I suppose that for such a simple query to an in-process sqlite it could well make a worthwhile difference. Have you seen significant improvements profiling similar cases?

In these patches though, I wasn't looking for optimization opportunities. My approach was to achieve compatibility with 1.3.2 with minimally invasive changes to the current codebase. If you want me to refactor along the patterns you described and re-submit I can, but my thought was to focus on functionality while navigating the genshi->jinja2 transition and assess performance bottlenecks once that stabilizes.

[Ryan J Ollos]

Note that I said coding style. There are no performance implications that I'm aware of.

[Jonathan Laufersweiler]

Is the form presented acceptable then? Most of the examples in the guide follow the with form rather than the for form. The only major difference I recognize is taking the environment passed from self.env rather than declaring it explicitly. Or am I missing something? Is the separate variable storing the query string now considered poor form?

[Ryan J Ollos]

Please consider using one of the condensed forms, at least for future patches. Consider that even the following reduces two unnecessary statements and retains use of the context manager:

with self.env.db_query as db:
    for row in db("SELECT * FROM milestone_version"):
        data.update({row[0]: row[1]})

In most cases it's just not necessary anymore to directly create a cursor.

List comprehensions are generally preferred as coding style in Trac for creating variables, which is the reason that I think the last form in comment:3 is most preferred.

On Trac trunk we can use dict comprehensions to make it even more readable:

data = {milestone: version for milestone, version in self.env.db_query("""
        SELECT milestone, version from milestone_version
        """)}

[Jonathan Laufersweiler]

Thank you for making that clear. I'll favor that idiom in the future.

I suggest also making that explicit in the above-linked TracDev reference. It was the first couple of examples there (which use cursor explicitly) that lead me to use that style as a least-change path to compatibility for existing code that's chock-full of the older form.

[Ryan J Ollos]

Replying to Jonathan Laufersweiler:

I suggest also making that explicit in the above-linked TracDev reference. It was the first couple of examples there (which use cursor explicitly) that lead me to use that style as a least-change path to compatibility for existing code that's chock-full of the older form.

I've also considered that all the historical discussion in the TracDev article can be confusing. It helps someone understand how we got to the form we have today, but in most cases the user would just like to know what's relevant today. I'll spend some time revising it.

[Ryan J Ollos]

In 17054:

SimpleMultiProject 0.6.0dev: Require Trac >= 1.0

Refs #13380.

[Ryan J Ollos]

In 17060:

SimpleMultiProject 0.6.0dev: Tidy codebase and remove deprecated code

Refs #13380.

[Ryan J Ollos]

In 17061:

SimpleMultiProject 0.6.0dev: Make compatible with latest Trac trunk

Refs #13380.

[Ryan J Ollos]

Please test in a non-production environment and let me know if any issues are found. The changes were pretty extensive so I expect testing will reveal more defects.

[Ryan J Ollos]

The "Group by project" filter on the roadmap results in a traceback:

File "/Users/rjollos/Documents/Workspace/trac-dev/teo-rjollos.git/trac/templates/genshi/progress_bar.html", line 35, in <Expression u"_('%(count)s/%(total)s %(title)s',                       count=interval.count, total=stats.count, title=interval.title)">          
  <a href="${interval_hrefs[idx] if interval_hrefs else None}"

I don't have a fix for that yet.

[Ryan J Ollos]

In 17062:

SimpleMultiProject 0.6.0dev: Refactor template modifications

Refs #13380.

[Ryan J Ollos]

In 17063:

SimpleMultiProject 0.6.0dev: Fix error in environment upgrade

Refs #13380.

[Ryan J Ollos]

In 17064:

SimpleMultiProject 0.6.0dev: Fix unintentional change in r17063

Refs #13380.

[Ryan J Ollos]

In 17065:

SimpleMultiProject 0.6.0dev: Fix unintentional change in r17064

Refs #13380.

[Ryan J Ollos]

Sorry for the mess in [17063:17065].

[Ryan J Ollos]

In 17066:

SimpleMultiProject 0.6.0dev: Fix association with multiple projects

Refs #13380.

[Ryan J Ollos]

In 17067:

SimpleMultiProject 0.6.0dev: Fix display of versions on milestone admin

Refs #13380.

[Ryan J Ollos]

In 17073:

SimpleMultiProject 0.6.0dev: Restore compatibility with Trac < 1.3.2

Refs #13380, Fixes #13392.

[anonymous]

I'm reporter of #13392, The milestone select box in ticket edit screen doesn't work properly when project is selected/changed. Even I use only Ascii character(I'm Japanese), still doesn't work.

[Ryan J Ollos]

Sorry for the delay. There are a few issues that need fixing and I'll return to it soon.

[Ryan J Ollos]

In 17166:

SimpleMultiProject 0.6.0dev: Fix project select form not changing milestone

Refs #13380.

[Ryan J Ollos]

Please let me know if any other issues are found. if not, I'll close the issue in a week or two.

[anonymous]

Thank you sir, I'm reporter of #13392. For the time being, this plugin works well. Thank you for response again.

[Ryan J Ollos]

In 17179:

SimpleMultiProject 0.6.0dev: Fix project components not shown

Patch by Martijn Zeedijk.

Refs #13380, Fixes #13429.

[Ryan J Ollos]

In 17180:

SimpleMultiProject 0.6.0dev: Fix project versions not shown

Patch by Martijn Zeedijk.

Refs #13380, Fixes #13431.

[Ryan J Ollos]

Replying to Jonathan Laufersweiler:

I suggest also making that explicit in the above-linked TracDev reference. It was the first couple of examples there (which use cursor explicitly) that lead me to use that style as a least-change path to compatibility for existing code that's chock-full of the older form.

​TracDev/DatabaseApi has been updated. If you have additional suggestions for the page, please raise a thread on the ​trac-dev mailing list.