Modify

Opened 13 years ago

Closed 12 years ago

#9216 closed defect (fixed)

Some errors following r10690, "KeyError"

Reported by: Ryan J Ollos Owned by: Chris Nelson
Priority: high Component: TracJsGanttPlugin
Severity: normal Keywords:
Cc: Trac Release: 0.11

Description

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.

Attachments (2)

DueDateISNotACustomTicketField.png (10.8 KB) - added by Ryan J Ollos 13 years ago.
th9216-r10704-0.11.7.patch (5.1 KB) - added by Ryan J Ollos 13 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 13 years ago by Ryan J Ollos

Summary: Some errors following r10690Some errors following r10690, "KeyError"

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

  • 0.11/tracjsgantt/tracjsgantt.py

     
    572572                if self.fields['finish'] and self.fields['parent']:
    573573                    pid = int(t[self.fields['parent']])
    574574                    # If this ticket has a parent, process it
    575                     if pid != 0:
     575                    if pid and pid in self.ticketsByID:
    576576                        parent = self.ticketsByID[pid]
    577577                        _schedule_task_alap(parent)
    578578                        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:

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

comment:3 in reply to:  1 Changed 13 years ago by Chris Nelson

Status: newassigned

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.

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

comment:5 Changed 13 years ago by Ryan J Ollos

Priority: normalhigh

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.

comment:6 Changed 13 years ago by 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)

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

     
    44from datetime import timedelta, datetime
    55from operator import itemgetter, attrgetter
    66
    7 from trac.util.datefmt import format_date
     7from trac.util.datefmt import format_date, utc
    88from trac.util.html import Markup
    99from trac.wiki.macros import WikiMacroBase
    1010from trac.web.chrome import Chrome
     
    777777                milestoneTicket['level'] = 0
    778778               
    779779                # If there's no due date, default to today at close of business
    780                 ts = row[1] or (datetime.today() + timedelta(hours=self.hpd))
     780                ts = row[1] or (datetime.now(utc) + timedelta(hours=self.hpd))
    781781                milestoneTicket[self.fields['finish']] = \
    782782                    format_date(ts, self.dbDateFormat)
    783783

Changed 13 years ago by Ryan J Ollos

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

     
    1212from trac.ticket.query import Query
    1313
    1414from trac.config import IntOption, Option
    15 from trac.core import implements, Component
     15from trac.core import implements, Component, TracError
    1616from trac.web.api import IRequestFilter
    1717from trac.web.chrome import ITemplateProvider, add_script, add_stylesheet
    1818from pkg_resources import resource_filename
     
    410410                    t[self.fields['parent']] = parent[1:]
    411411
    412412        for t in self.tickets:
    413413            # Clean up custom fields which might be null ('--') vs. blank ('')
    414414            nullable = [ 'pred', 'succ',
    415415                         'start', 'finish',
    416416                         'parent',
    417417                         'worked', 'estimate', 'percent' ]
    418418            for field in nullable:
    419                 if self.fields.get(field) and t[self.fields[field]] == '--':
     419                if self.fields[field] and self.fields[field] not in t:
     420                    raise TracError( "%s is not a custom ticket field" % self.fields[field] )
     421               
     422                if self.fields[field] and t[self.fields[field]] == '--':
    420423                    t[self.fields[field]] = ''
    421424
    422425            # Get list of children

This leads to a nice error:

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

Changed 13 years ago by Ryan J Ollos

Attachment: th9216-r10704-0.11.7.patch added

comment:10 in reply to:  9 ; Changed 13 years ago by 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.

comment:11 in reply to:  10 Changed 13 years ago by 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'
>>> 

comment:12 Changed 13 years ago by Chris Nelson

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

Applying a patch from rjollos. Thanks!

comment:13 in reply to:  12 Changed 13 years ago by 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.

comment:14 Changed 13 years ago by Ryan J Ollos

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

comment:15 Changed 12 years ago by Ryan J Ollos

Resolution: fixed
Status: assignedclosed

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Chris Nelson.
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.