Modify

Opened 4 years ago

Closed 4 years ago

Last modified 3 years ago

#13380 closed enhancement (fixed)

Patches for 1.3.2+ compatibility

Reported by: Jonathan Laufersweiler Owned by: Ryan J Ollos
Priority: normal Component: SimpleMultiProjectPlugin
Severity: normal Keywords: patch
Cc: Trac Release: 1.3

Description

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.

Attachments (8)

0001-Trac-1.3.2-Compatability.patch (9.5 KB) - added by Jonathan Laufersweiler 4 years ago.
0002-Trac-1.3.2-Compatability.patch (4.4 KB) - added by Jonathan Laufersweiler 4 years ago.
0003-Trac-1.3.2-Compatability.patch (1.6 KB) - added by Jonathan Laufersweiler 4 years ago.
0004-Trac-1.3.2-Compatability.patch (2.8 KB) - added by Jonathan Laufersweiler 4 years ago.
0005-Trac-1.3.2-Compatability.patch (18.3 KB) - added by Jonathan Laufersweiler 4 years ago.
0006-Trac-1.3.2-Compatability.patch (2.0 KB) - added by Jonathan Laufersweiler 4 years ago.
0008-Trac-1.3.2-Compatability.patch (9.0 KB) - added by Jonathan Laufersweiler 4 years ago.
Trac-1.3.2-Compatability.diff (50.9 KB) - added by Jonathan Laufersweiler 4 years ago.

Download all attachments as: .zip

Change History (40)

Changed 4 years ago by Jonathan Laufersweiler

Changed 4 years ago by Jonathan Laufersweiler

Changed 4 years ago by Jonathan Laufersweiler

Changed 4 years ago by Jonathan Laufersweiler

Changed 4 years ago by Jonathan Laufersweiler

Changed 4 years ago by Jonathan Laufersweiler

Changed 4 years ago by Jonathan Laufersweiler

Changed 4 years ago by Jonathan Laufersweiler

comment:1 Changed 4 years ago by Ryan J Ollos

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

comment:2 Changed 4 years ago by 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.

comment:3 Changed 4 years ago by 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.

Last edited 4 years ago by Ryan J Ollos (previous) (diff)

comment:4 Changed 4 years ago by 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.

comment:5 Changed 4 years ago by Ryan J Ollos

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

comment:6 Changed 4 years ago by 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?

comment:7 Changed 4 years ago by 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
        """)}

comment:8 Changed 4 years ago by 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.

comment:9 in reply to:  8 Changed 4 years ago by 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.

comment:10 Changed 4 years ago by Ryan J Ollos

Owner: changed from falkb to Ryan J Ollos
Status: newaccepted
Summary: Patches for 1.3.2+ Compatability.Patches for 1.3.2+ compatibility

comment:11 Changed 4 years ago by Ryan J Ollos

In 17054:

SimpleMultiProject 0.6.0dev: Require Trac >= 1.0

Refs #13380.

comment:12 Changed 4 years ago by Ryan J Ollos

In 17060:

SimpleMultiProject 0.6.0dev: Tidy codebase and remove deprecated code

Refs #13380.

comment:13 Changed 4 years ago by Ryan J Ollos

In 17061:

SimpleMultiProject 0.6.0dev: Make compatible with latest Trac trunk

Refs #13380.

comment:14 Changed 4 years ago by 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.

comment:15 Changed 4 years ago by 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.

comment:16 Changed 4 years ago by Ryan J Ollos

In 17062:

SimpleMultiProject 0.6.0dev: Refactor template modifications

Refs #13380.

comment:17 Changed 4 years ago by Ryan J Ollos

In 17063:

SimpleMultiProject 0.6.0dev: Fix error in environment upgrade

Refs #13380.

comment:18 Changed 4 years ago by Ryan J Ollos

In 17064:

SimpleMultiProject 0.6.0dev: Fix unintentional change in r17063

Refs #13380.

comment:19 Changed 4 years ago by Ryan J Ollos

In 17065:

SimpleMultiProject 0.6.0dev: Fix unintentional change in r17064

Refs #13380.

comment:20 Changed 4 years ago by Ryan J Ollos

Sorry for the mess in [17063:17065].

comment:21 Changed 4 years ago by Ryan J Ollos

In 17066:

SimpleMultiProject 0.6.0dev: Fix association with multiple projects

Refs #13380.

comment:22 Changed 4 years ago by Ryan J Ollos

In 17067:

SimpleMultiProject 0.6.0dev: Fix display of versions on milestone admin

Refs #13380.

comment:23 Changed 4 years ago by Ryan J Ollos

In 17073:

SimpleMultiProject 0.6.0dev: Restore compatibility with Trac < 1.3.2

Refs #13380, Fixes #13392.

comment:24 Changed 4 years ago by 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.

comment:25 Changed 4 years ago by Ryan J Ollos

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

comment:26 Changed 4 years ago by Ryan J Ollos

In 17166:

SimpleMultiProject 0.6.0dev: Fix project select form not changing milestone

Refs #13380.

comment:27 Changed 4 years ago by 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.

comment:28 Changed 4 years ago by anonymous

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

comment:29 Changed 4 years ago by Ryan J Ollos

In 17179:

SimpleMultiProject 0.6.0dev: Fix project components not shown

Patch by Martijn Zeedijk.

Refs #13380, Fixes #13429.

comment:30 Changed 4 years ago by Ryan J Ollos

In 17180:

SimpleMultiProject 0.6.0dev: Fix project versions not shown

Patch by Martijn Zeedijk.

Refs #13380, Fixes #13431.

comment:31 Changed 4 years ago by Ryan J Ollos

Resolution: fixed
Status: acceptedclosed

comment:32 in reply to:  8 Changed 3 years ago by 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.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted. Next status will be 'reopened'.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.