Some errors following r10690, "KeyError"

[Ryan J Ollos]

I'm stepping through your changes, and with [10690], I get the following error:

Error: Macro TracJSGanttChart(milestone=M1 - I10, lwidth=600, colorBy=owner, expandClosedTickets=0, startDate=0, endDate=0, res=0) failed
type object 'datetime.datetime' has no attribute 'datetime'

If I step forward through the changesets, I start to get different errors.

With r10701, I get:

2011-09-28 16:51:19,548 Trac[formatter] ERROR: Macro TracJSGanttChart(milestone=M1 - I10, lwidth=600, colorBy=owner, expandClosedTickets=0, startDate=0, endDate=0, res=0) failed:
Traceback (most recent call last):
  File "/usr/lib/python2.6/site-packages/Trac-0.11.7-py2.6.egg/trac/wiki/formatter.py", line 484, in _macro_formatter
    return macro.process(args, in_paragraph=True)
  File "/usr/lib/python2.6/site-packages/Trac-0.11.7-py2.6.egg/trac/wiki/formatter.py", line 180, in process
    text = self.processor(text)
  File "/usr/lib/python2.6/site-packages/Trac-0.11.7-py2.6.egg/trac/wiki/formatter.py", line 167, in _macro_processor
    text)
  File "/usr/lib/python2.6/site-packages/Trac_jsGantt-0.9_r10701-py2.6.egg/tracjsgantt/tracjsgantt.py", line 1077, in expand_macro
    tasks = self._add_tasks(options)
  File "/usr/lib/python2.6/site-packages/Trac_jsGantt-0.9_r10701-py2.6.egg/tracjsgantt/tracjsgantt.py", line 1039, in _add_tasks
    self._schedule_tasks(options)
  File "/usr/lib/python2.6/site-packages/Trac_jsGantt-0.9_r10701-py2.6.egg/tracjsgantt/tracjsgantt.py", line 706, in _schedule_tasks
    _schedule_task_alap(t)
  File "/usr/lib/python2.6/site-packages/Trac_jsGantt-0.9_r10701-py2.6.egg/tracjsgantt/tracjsgantt.py", line 602, in _schedule_task_alap
    finish = _earliest_successor(t, _ancestor_finish(t))
  File "/usr/lib/python2.6/site-packages/Trac_jsGantt-0.9_r10701-py2.6.egg/tracjsgantt/tracjsgantt.py", line 576, in _ancestor_finish
    parent = self.ticketsByID[pid]
KeyError: 451

Ticket 451 is not part of my Trac milestone "M1 - I10", but there are two tickets in "M1 - I10" that have 451 as a parent.

I'll dig deeper into this sometime in the next few days.

[Ryan J Ollos]

I don't see the error if I patch the code as follows:

0.11/tracjsgantt/tracjsgantt.py

@@ -572 +572 @@
                 if self.fields['finish'] and self.fields['parent']:
                     pid = int(t[self.fields['parent']])
                     # If this ticket has a parent, process it
-                    if pid != 0:
+                    if pid and pid in self.ticketsByID:
                         parent = self.ticketsByID[pid]
                         _schedule_task_alap(parent)
                         finish = self.ticketsByID[pid]['calc_finish']

With this change, it looks to me like the parent will not be scheduled if the parent is not in ticketsByID. This appears to cause the scheduling of parents that are not part of the milestone to be skipped, which may be the behavior that we want.

Btw, you had asked about code review in another ticket, so I'll make a minor comment here. I've not done an incredible amount of coding in Python, but from what I've seen, it seems the following is preferred:

if pid

rather than:

if pid != 0

Similarly, the following is preferred:

if not t.get('calc_finish'):

rather than:

if t.get('calc_finish') == None:

[Chris Nelson]

(In [10703]) Handle parents not in chart when computing start, finish. Refs #9216.

This may not be the perfect solution (we want to use that dependency) but pulling in dependent tickets is a much bigger change that will wait.

[Chris Nelson]

Replying to rjollos:

I don't see the error if I patch the code as follows: ... With this change, it looks to me like the parent will not be scheduled if the parent is not in ticketsByID. This appears to cause the scheduling of parents that are not part of the milestone to be skipped, which may be the behavior that we want.

Fixed. Sorry. And thanks.

Btw, you had asked about code review in another ticket, so I'll make a minor comment here. I've not done an incredible amount of coding in Python, but from what I've seen, it seems the following is preferred:

if pid

rather than:

if pid != 0

In the plugin I sometimes need to distinguish between, for example, an value that isn't set (None) and one that is set to 0. I'll look through with your note in mind, though. Thanks.

Similarly, the following is preferred:

if not t.get('calc_finish'):

rather than:

if t.get('calc_finish') == None:

OK.

[Chris Nelson]

(In [10704]) Friendlier handling of missing tickets. Refs #9216.

Log a message when a parent, predecessor, or successor is not in the chart.

[Ryan J Ollos]

Okay, new error now:

2011-09-29 12:29:23,801 Trac[formatter] ERROR: Macro TracJSGanttChart(milestone=M1 - I10, lwidth=600, colorBy=owner, expandClosedTickets=0, startDate=0, endDate=0, res=0) failed:
Traceback (most recent call last):
  File "/usr/lib/python2.6/site-packages/Trac-0.11.7-py2.6.egg/trac/wiki/formatter.py", line 484, in _macro_formatter
    return macro.process(args, in_paragraph=True)
  File "/usr/lib/python2.6/site-packages/Trac-0.11.7-py2.6.egg/trac/wiki/formatter.py", line 180, in process
    text = self.processor(text)
  File "/usr/lib/python2.6/site-packages/Trac-0.11.7-py2.6.egg/trac/wiki/formatter.py", line 167, in _macro_processor
    text)
  File "build/bdist.linux-x86_64/egg/tracjsgantt/tracjsgantt.py", line 1103, in expand_macro
    tasks = self._add_tasks(options)
  File "build/bdist.linux-x86_64/egg/tracjsgantt/tracjsgantt.py", line 1065, in _add_tasks
    self._schedule_tasks(options)
  File "build/bdist.linux-x86_64/egg/tracjsgantt/tracjsgantt.py", line 732, in _schedule_tasks
    _schedule_task_alap(t)
  File "build/bdist.linux-x86_64/egg/tracjsgantt/tracjsgantt.py", line 615, in _schedule_task_alap
    finish = _earliest_successor(t, _ancestor_finish(t))
  File "build/bdist.linux-x86_64/egg/tracjsgantt/tracjsgantt.py", line 584, in _ancestor_finish
    (t['id'], pid, pid))
TypeError: not all arguments converted during string formatting

I'll hopefully have another patch for you shortly.

[Ryan J Ollos]

I've setup a dev environment and see another error different from that described in comment:5. This error occurs when setting no options in trac.ini for the JsGanttChart macro. The patch will be provided in the next comment.

01:52:35 PM Trac[formatter] ERROR: Macro TracJSGanttChart(milestone=milestone2) failed: 
Traceback (most recent call last):
  File "/home/rjollos/Workspace/th9216/trac-0.11-stable/trac/wiki/formatter.py", line 491, in _macro_formatter
    return macro.process(args, in_paragraph=True)
  File "/home/rjollos/Workspace/th9216/trac-0.11-stable/trac/wiki/formatter.py", line 180, in process
    text = self.processor(text)
  File "/home/rjollos/Workspace/th9216/trac-0.11-stable/trac/wiki/formatter.py", line 167, in _macro_processor
    text)
  File "/home/rjollos/Workspace/tracjsganttplugin/0.11/tracjsgantt/tracjsgantt.py", line 1108, in expand_macro
    tasks = self._add_tasks(options)
  File "/home/rjollos/Workspace/tracjsganttplugin/0.11/tracjsgantt/tracjsgantt.py", line 1062, in _add_tasks
    self._add_milestones(options)
  File "/home/rjollos/Workspace/tracjsganttplugin/0.11/tracjsgantt/tracjsgantt.py", line 787, in _add_milestones
    format_date(ts, self.dbDateFormat)
  File "/home/rjollos/Workspace/th9216/trac-0.11-stable/trac/util/datefmt.py", line 143, in format_date
    return format_datetime(t, format, tzinfo=tzinfo)
  File "/home/rjollos/Workspace/th9216/trac-0.11-stable/trac/util/datefmt.py", line 111, in format_datetime
    t = to_datetime(t, tzinfo).astimezone(tz)

[Ryan J Ollos]

Grepping in the trac codebase shows no uses of datetime.today() and many uses of datetime.now(utc) to generate aware (non-naive) datetime objects. The project or user specific timezone setting is then applied in the datefmt module (in this case, by the function format_date).

0.11/tracjsgantt/tracjsgantt.py

@@ -4 +4 @@
 from datetime import timedelta, datetime
 from operator import itemgetter, attrgetter
 
-from trac.util.datefmt import format_date
+from trac.util.datefmt import format_date, utc
 from trac.util.html import Markup
 from trac.wiki.macros import WikiMacroBase
 from trac.web.chrome import Chrome
@@ -777 +777 @@
                 milestoneTicket['level'] = 0
                 
                 # If there's no due date, default to today at close of business
-                ts = row[1] or (datetime.today() + timedelta(hours=self.hpd))
+                ts = row[1] or (datetime.now(utc) + timedelta(hours=self.hpd))
                 milestoneTicket[self.fields['finish']] = \
                     format_date(ts, self.dbDateFormat)
 

[Ryan J Ollos]

I've run into another problem along the way, and it is not necessarily an issue with your plugin, but we might be able to improve the behavior slightly.

I installed the SubticketsPlugin and this configuration:

[ticket-custom]
startdate = text
startdate.label = Start Date
startdate.format = plain
duedate = text
duedate.label = Due Date
duedate.format = plain

[trac-jsgantt]
fields.parent = parents
fields.start = startdate
fields.finish = duedate

Initially though, I named the custom ticket field in camelcase, e.g. StartDate, and Trac seemed to be storing it as all lowercase. This led to the JsGanttChart not finding the field fields.start = StartDate, and there was no error checking in place for this, so I've added that to help with debugging an installation.

0.11/tracjsgantt/tracjsgantt.py

@@ -12 +12 @@
 from trac.ticket.query import Query
 
 from trac.config import IntOption, Option
-from trac.core import implements, Component
+from trac.core import implements, Component, TracError
 from trac.web.api import IRequestFilter
 from trac.web.chrome import ITemplateProvider, add_script, add_stylesheet
 from pkg_resources import resource_filename
@@ -410 +410 @@
                     t[self.fields['parent']] = parent[1:]
 
         for t in self.tickets:
             # Clean up custom fields which might be null ('--') vs. blank ('')
             nullable = [ 'pred', 'succ', 
                          'start', 'finish', 
                          'parent', 
                          'worked', 'estimate', 'percent' ]
             for field in nullable:
-                if self.fields.get(field) and t[self.fields[field]] == '--':
+                if self.fields[field] and self.fields[field] not in t:
+                    raise TracError( "%s is not a custom ticket field" % self.fields[field] )
+                
+                if self.fields[field] and t[self.fields[field]] == '--':
                     t[self.fields[field]] = ''
 
             # Get list of children

This leads to a nice error:

[Ryan J Ollos]

Okay, I'm back to working on the error described in comment:5 now. The issue there is with [10704]. I'm pretty sure + is not a valid string concatenation operator in Python. I do this same thing all the time since I work in C++ most often ... the fun of working in multiple languages ;)

I'll submit a net patch that deals with comment:5, comment:7 and comment:8. If you are able to commit this by tomorrow, I can pull the changes down on my server and test the original issue reported in this ticket.

[Ryan J Ollos]

Replying to rjollos:

I'm pretty sure + is not a valid string concatenation operator in Python ...

Stupid statement on my part ... + is of course valid for all built-in types, but it seems you can't use it in a string formatting statement:

>>> s =  "a b %s" + " d e %s" % ("c","f")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: not all arguments converted during string formatting

Anyway, the multi-line string as used in the patch seems to be more commonplace in Python.

[Chris Nelson]

Replying to rjollos:

Replying to rjollos:

I'm pretty sure + is not a valid string concatenation operator in Python ...

Stupid statement on my part ... + is of course valid for all built-in types, but it seems you can't use it in a string formatting statement:

>>> s =  "a b %s" + " d e %s" % ("c","f")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: not all arguments converted during string formatting

Anyway, the multi-line string as used in the patch seems to be more commonplace in Python.

This has bothered me before and you prompted me to finally figure it out. The problem is that % has higher precedence than + so

s = 'x:%d' + ' y:%d' % (x, y)

gets processed as

s = 'x:%d' + (' y:%d' % (x, y))

and there are too many substitutions for the second string and not enough for the first. We can either reorder the arguments or group the concatenated strings explicitly:

Python 2.6.5 (r265:79063, Apr 16 2010, 13:57:41) 
[GCC 4.4.3] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> x = 1
>>> y = 2
>>> s = "x:%d" % x + " y:%d" % y
>>> s
'x:1 y:2'
>>> s = ("x:%d" + " y:%d") % (x, y)
>>> s
'x:1 y:2'
>>> 

[Chris Nelson]

(In [10710]) Fix several minor errors in commits for scheduling. Refs #9216, #9024, #8982.

Applying a patch from rjollos. Thanks!

[Chris Nelson]

Replying to ChrisNelson:

(In [10710]) Fix several minor errors in commits for scheduling. Refs #9216, #9024, #8982.

Applying a patch from rjollos. Thanks!

Sorry, 9024 should have been #9042.

[Ryan J Ollos]

Okay, I'm up and running with r10710 on Trac 0.11.7 now. I think this ticket can be closed.