Modify

Opened 3 years ago

Closed 3 years ago

#9216 closed defect (fixed)

Some errors following r10690, "KeyError"

Reported by: rjollos Owned by: ChrisNelson
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 rjollos 3 years ago.
th9216-r10704-0.11.7.patch (5.1 KB) - added by rjollos 3 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 follow-up: Changed 3 years ago by rjollos

  • Summary changed from Some errors following r10690 to Some 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 3 years ago by ChrisNelson

(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 3 years ago by ChrisNelson

  • Status changed from new to assigned

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 3 years ago by ChrisNelson

(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 3 years ago by rjollos

  • Priority changed from normal to high

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 3 years ago by rjollos

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 3 years ago by rjollos

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 3 years ago by rjollos

comment:8 Changed 3 years ago by rjollos

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 follow-up: Changed 3 years ago by rjollos

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 3 years ago by rjollos

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

comment:11 in reply to: ↑ 10 Changed 3 years ago by ChrisNelson

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 follow-up: Changed 3 years ago by ChrisNelson

(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 3 years ago by ChrisNelson

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 3 years ago by rjollos

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

comment:15 Changed 3 years ago by rjollos

  • Resolution set to fixed
  • Status changed from assigned to closed

Add Comment

Modify Ticket

Action
as closed .
as The resolution will be set. Next status will be 'closed'.
to The owner will be changed from ChrisNelson. Next status will be 'closed'.
The resolution will be deleted. Next status will be 'reopened'.
Author


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

 
Note: See TracTickets for help on using tickets.