Christopher Lenz wrote:
> Am 11.09.2007 um 10:25 schrieb Christian Boos:
>
>> I'd like to reach a decision for ticket #5572 (Roadmap Progress bar
>> errors when using custom Workflow). Again a bug report that turned
>> into
>> a new feature ;-)
>>
> [snip]
>
>> Opinions?
>>
>
> I agree with you in general, but maybe we can still simplify the
> change a bit.
>
> Specifically, do we really need the wildcard and negation support for
> specifying the list of statuses? People won't have 50 different
> statuses (and if they do, they deserve some punishment ;) ), and you
> really don't need to go in and edit the grouping configuration all
> the time. So I think that using just a plain list of status names
> would be sufficient, and simplify the code (and documentation). That
> would also remove the reliance on the order of the options if I'm not
> mistaken.
>
>
Negation support was there for being able to specify things like
"anything but closed".
A wildcard is useful for specifying a catch-all group, useful when you
upgraded the workflow a few times and you are unsure about what are the
remaining states.
However the first use case could probably be subsumed by the second and
we'd only need one catch-all group after all.
So yes, I think the patch can be simplified further in this area.
The .order is still needed I think, in order to have control over the
sequence of the groups within the progress bar.
> Some other minor nits:
>
> * Naming of the "all_status" and "remaining_status" variables: I'd
> prefer the more common plural of "statuses" here :)
>
> * The .css option: I think "css_class" would be more intuitive/explicit
>
> * The .args option: Not sure about this one... first, the name should
> probably be "query_args" to be more descriptive. And that you
> separate options with a comma is not really intuitive, as it looks
> like a query string; I would've first tried using "&" there :P But
> that's something that can be fixed by a slight change to the
> documentation.
>
All good points. I'm surprised Eli didn't bug me for the first one,
considering how I harassed him about the same thing on the workflow
branch ;-)
I've uploaded an updated patch on the ticket, which applies cleanly on
r6000/trunk:
http://trac.edgewall.org/attachment/ticket/5572/milestone_groups-r6000.diff
However, for ease of review I'm also attaching a diff to this mail
showing the requested changes (i.e. that one would apply on top of the
previous patch).
This would nearly have deserved a branch, right? :-)
-- Christian
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups "Trac
Development" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to [EMAIL PROTECTED]
For more options, visit this group at
http://groups.google.com/group/trac-dev?hl=en
-~----------~----~----~----~------~----~------~--~---
diff -r 7547da304fd3 -r 0647ea041f94 trac/ticket/roadmap.py
--- a/trac/ticket/roadmap.py Tue Sep 11 16:26:24 2007 +0200
+++ b/trac/ticket/roadmap.py Tue Sep 11 17:11:41 2007 +0200
@@ -138,14 +138,14 @@ class DefaultTicketGroupStatsProvider(Co
# Qualifiers for the above group (the group must have been defined first):
closed.order = 0 # sequence number in the progress bar
- closed.args = group=resolution # optional extra param for the query
+ closed.query_args = group=resolution # optional extra param for the query
closed.overall_completion = true # count for overall completion
# Definition of an 'active' group:
- active = *
+ active = * # one catch-all group is allowed
active.order = 1
- active.css = open # CSS class for this interval
+ active.css_class = open # CSS class for this interval
# The CSS class can be one of: new (yellow), open (no color) or
# closed (green). New styles can easily be added using the following
@@ -157,13 +157,13 @@ class DefaultTicketGroupStatsProvider(Co
default_milestone_groups = [
{'name': 'closed', 'status': 'closed',
- 'args': 'group=resolution', 'overall_completion': 'true'},
- {'name': 'active', 'status': '*', 'css': 'open'}
+ 'query_args': 'group=resolution', 'overall_completion': 'true'},
+ {'name': 'active', 'status': '*', 'css_class': 'open'}
]
def _get_ticket_groups(self):
- """Returns a dict describing the ticket groups used in milestone
- progress bars.
+ """Returns a list of dict describing the ticket groups
+ in the expected order of appearance in the milestone progress bars.
"""
if 'milestone-groups' in self.config:
groups = {}
@@ -183,9 +183,9 @@ class DefaultTicketGroupStatsProvider(Co
def get_ticket_group_stats(self, ticket_ids):
total_cnt = len(ticket_ids)
- all_status = set(TicketSystem(self.env).get_all_status())
+ all_statuses = set(TicketSystem(self.env).get_all_status())
status_cnt = {}
- for s in all_status:
+ for s in all_statuses:
status_cnt[s] = 0
if total_cnt:
cursor = self.env.get_db_cnx().cursor()
@@ -197,42 +197,50 @@ class DefaultTicketGroupStatsProvider(Co
status_cnt[s] = cnt
stat = TicketGroupStats('ticket status', 'ticket')
- remaining_status = set(all_status)
- for group in self._get_ticket_groups():
- group_cnt = 0
+ remaining_statuses = set(all_statuses)
+ groups = self._get_ticket_groups()
+ catch_all_group = None
+ # we need to go through the groups twice, so that the catch up group
+ # doesn't need to be the last one in the sequence
+ for group in groups:
status_str = group['status'].strip()
- invert = False
if status_str == '*':
- group_status = remaining_status
- remaining_status = set()
+ if catch_all_group:
+ raise TracError(_(
+ "'%(group1)s' and '%(group2)s' milestone groups "
+ "both are declared to be \"catch-all\" groups. "
+ "Please check your configuration.",
+ group1=group['name'], group2=catch_all_group['name']))
+ catch_all_group = group
else:
- if status_str.startswith('!'):
- status_str = status_str[1:]
- invert = True
- group_status = set([s.strip() for s in status_str.split(',')])\
- & all_status
- if invert:
- remaining_status = group_status # see XOR trick below
- elif group_status - remaining_status:
+ group_statuses = set([s.strip()
+ for s in status_str.split(',')]) \
+ & all_statuses
+ if group_statuses - remaining_statuses:
raise TracError(_(
"'%(groupname)s' milestone group reused status "
"'%(status)s' already taken by other groups. "
"Please check your configuration.",
groupname=group['name'],
- status=', '.join(group_status - remaining_status)))
+ status=', '.join(group_statuses - remaining_statuses)))
else:
- remaining_status -= group_status
+ remaining_statuses -= group_statuses
+ group['statuses'] = group_statuses
+ if catch_all_group:
+ catch_all_group['statuses'] = remaining_statuses
+ for group in groups:
+ group_cnt = 0
query_args = {}
for s, cnt in status_cnt.iteritems():
- if (s in group_status) ^ invert:
+ if s in group['statuses']:
group_cnt += cnt
query_args.setdefault('status', []).append(s)
- for arg in [kv for kv in group.get('args', '').split(',')
+ for arg in [kv for kv in group.get('query_args', '').split(',')
if '=' in kv]:
k, v = [a.strip() for a in arg.split('=', 1)]
query_args[k] = v
stat.add_interval(group['name'], group_cnt, query_args,
- group.get('css', group['name']),
+ group.get('css_class', group['name']),
bool(group.get('overall_completion')))
stat.refresh_calcs()
return stat
