Progress toward Budget module

[Derek Atkins]

Darin Willits <darin at willits.ca> writes:

> On August 5, 2004 06:34 pm, Derek Atkins wrote:
>> A few comments..
>>
>> 1) Your changes to app-utils/gnc-ui-common.h and overrides/gnucash
>>    are unwarranted.  Also, you've created a commodity.glade in gnome/glade
>>    that conflicts with the commodity.glade in gnome-utils.
>
> Crap.  I remember this.  For some reason my environment was not set up 
> properly after running ./configure such that the source tree wasn't building.  
> I was meaning to track down/figure this out further but got caught up in 
> coding.  It was just easier for me to comment this stuff out.  sorry, my bad.

Hehe..  Happens to the best of us.  No harm, no foul.

>> 2) You probably shouldn't use xaccXXX for new names (e.g. xaccBudgetDef)
>
> This was an attempt on my part to adhere to the given coding/naming standards 
> as is discussed in one of the coding guideline docs... can't find it now of 
> course.  :-)  Maybe this doc should reflect the new preferred naming 
> conventions if it doesn't already.

I think there are probably way too many docs involved..  At this point
we're trying to "deprecate" the xacc names..  New names should use
gnc, Gnc, gncp, etc.

>> 3) I'm not convinced that gnc_book_get_budgets() (and others of that
>>    ilk) is the right naming convention for your functions...
>
> Again modeled after existing code... from Scheduled Transactions I believe.

Yea, this is a hard problem -- Generally the functions should be

  gnc_<obj1>_<operation...> (<obj1-type>, ...)

So, gnc_book_...(QofBook*, ...) would be an operation on a book..
HOWEVER, generally this code would be in the "book" source file.

The question here is when you're adding a budget to a book, are you
operating on the budget or operating on the book?  Tough question to
answer.  It's also important to consider who gets events in this
case?

Mostly my concern here was the fact that you have functions named
gnc_book_blah() defined in gnc-budget which, when trying to find the
source code later is less-than-intuitive.  Granted, "find" can work
wonders, but generally when defining an object module it's best to
name functions in that "object's" namespace.

Not a major issue, but just something to keep in mind (and perhaps
clean up later).  Perhaps the add/remove shouldn't be exposed -- only
the 'create' and 'delete' functions should need to add/remove the
budgets from a book?  Then you can rename them to
gnc_budget_add_to_book()?  *shrugs*

>> 4) config.h should never be included in a header file, only in .c files...
>>    This is because config.h is not installed, but the header files are.
>
> Oops.  how did that get in there.

no clue  :)

>> 5) I'm also unclear that you want to be part of MOD_GUI (e.g.
>> gnc-budget-cat.c) 6) I'm wondering if there are other budget objects that
>> want to be QOFified in order to "store" the data..
>>
>
> Used to ease debugging as I could just turn on/off messages from my part of 
> the engine/gui without getting all the rest.  Should have taken that out.  I 
> will do that and send an update.

Ok.

>> Having said that, it looks good, so I'll try to commit this to CVS
>> later today.  I'll make sure not to commit the pieces I mention in #1
>> (but you might want to check your source tree.  I'll let you fix the
>> rest on your own later.  :)
>>
>
> Excellent!  Once it is in the main tree I will fix some of the above issues 
> and send another patch so that we are working with a smaller change set.  

Great.  Thanks.  I'll get to it within a few hours.  But first -- lunch.  :)

> Cheers,
>
> Darin

-derek

-- 
       Derek Atkins, SB '93 MIT EE, SM '95 MIT Media Laboratory
       Member, MIT Student Information Processing Board  (SIPB)
       URL: http://web.mit.edu/warlord/    PP-ASEL-IA     N1NWH
       warlord at MIT.EDU                        PGP key available