On Mon, Nov 17, 2008 at 02:52:00AM +0000, Lee Baylis wrote:
> Hi All,
>
> Apologies in advance for the essay!
>
> Lee Baylis wrote:
>>> I would also like to extend the mrp-resource data schema with a 'maximum
>>> allocatable
>>> units' field definable on each resource.
What is the definition of this number of units and how does it differ
from the time that a resource has at a certain moment according to its
calendar?
> Only the option on the project to set overload behaviour is left, so I will
> use a flag for now, then look at adding it to the schema when I start
> introducing algorithms to choose from - at that point I will have a better
> idea as to what behaviour this option will need to hold.
I'm not sure what you mean by flag. Do you mean a custom project
property?
> In the interests of submitting smaller steps, I've attached the completed
> patches from just that piece of work, but note that no new user
> functionality is introduced by incorporating these patches into planner -
> they just set the stage for the next steps.
I reviewed everything and in general it looks pretty good. Most of my
comments below are about rather minor issues. Most of them I would just
fix myself before committing, but I'll mention them anyway. The number
in front is the line number.
I also have two general remarks.
The first is that units_in_use is sort of a caching variable. There are
times when the values stored in them are not up-to-date. Some functions
have as a precondition that those values are up-to-date, so I would like
to have them verify that precondition. To enable them to do that, some
other functions should 'invalidate' that cache somehow.
My other remark is about the function description comments. Please break
up those comment lines when they become too long.
> planner-usage-row:
1073: useless return
> mrp-assignment:
276: this function is hard to read (because of things like using the
enum in a subtraction and as a boolean condition). I think it can be
made more readable and faster at the same time by not splitting out the
'common' part of mrp_assignment_edges_remove_to_last_before and
mrp_assignment_edges_remove_from_first_after.
mrp_assignment_edges_remove_to_last_before could be:
if (edges) {
while (edges && edges->data < time) {
edges = g_list_delete_link(edges, edges);
}
}
return edges;
and then mrp_assignment_edges_remove_from_first_after would look like:
if (edges) {
edges = g_list_reverse(edges);
while (edges && edges->data < time) {
edges = g_list_delete_link(edges, edges);
}
edges = g_list_reverse(edges);
}
return edges;
455: variable old_units is used for both old and new values, so call it
units instead
mrp-assignment.h
74: indentation is off just before the param list
It's off in a lot of places, but those weren't introduced by you.
I do accept patches for indentation fixes if you want to clean stuff
up before you make changes, but such patches should contain _only_
whitespace fixes.
> mrp-resource:
95: indentation is off inside the param list
911: I don't see why mrp_resource_test_if_overallocated() should exist.
The MrpResourceAllocation enum is visible from the same places as this
function is, so people could just call mrp_resource_allocation_status
and compare.
954: typo: resouce
I'll comment on the rest of you message later.
Regards,
Maurice.
--
Maurice van der Pot
Gentoo Linux Developer griffon26 gentoo org http://www.gentoo.org
Gnome Planner Developer griffon26 kfk4ever com http://live.gnome.org/Planner
Attachment:
pgpfY5qbmDxyY.pgp
Description: PGP signature