GNOME Bugzilla – Bug 635469
[PATCH] Improve qofquery bindings
Last modified: 2018-06-29 22:47:57 UTC
Created attachment 174992 [details] [review] Add bindings for qofqueries on invoices (patch against trunk r19847) (Reporting this as "Engine", since there's no general "Bindings" component, only "Python Bindings") I'm attaching two patches to improve the qofquery bindings. I am using these in a custom report. The first patch adds qofquery bindings for querying invoices, the second adds a binding for qof_query_add_term.
Created attachment 174993 [details] [review] Add binding for qof_query_add_term (patch against trunk r19847)
Comment on attachment 174992 [details] [review] Add bindings for qofqueries on invoices (patch against trunk r19847) Patch is fine. Two minor cosmetic things: I'd suggest to start the new type identifier with a capital letter (i.e. GncInvoiceList). And I'd like to know how to apply your submitted patch file in git so that I already get your commit message transferred correctly - obviously you didn't use "git format-patch" where I would have used "git am", or which git command sequence should I use here?
Created attachment 175011 [details] [review] Add bindings for qofqueries on invoices (patch against trunk r19847, updated patch) I've updated this patch to use the proper capitalized typedef name. As for the patch generation, I just used "git show" for that, since git format-patch only creates mbox format (and I wasn't intending to email the patches). Apparently you're using git as well, so I'll just use the email format from now on. I'll also update the second patch to git format-patch format next. Btw, compliments on the quick responses to my patches. It's very encouraging to patch authors, so kudos. And expect some more patches soon :-)
Created attachment 175012 [details] [review] Add binding for qof_query_add_term (patch against trunk r19847, generated with git-format-patch)
Comment on attachment 175012 [details] [review] Add binding for qof_query_add_term (patch against trunk r19847, generated with git-format-patch) (Just a curious SWIG question: Is it really necessary to add an explicit declaration into engine.i to make sure the correct typemap is being used? I recall in some cases the typemaps from swig are already applied on the "normal" C declarations. If that is possible by replacing a GList* argument in the "normal" headers by some typedef'd type (i.e. GSList*), feel free to use those instead. Avoiding the code duplication is surely better than having to include it. But the patch itself is fine.)
Created attachment 175060 [details] [review] 0001-Add-a-typemap-for-a-QofQueryParamList-typedef.patch It's probably not necessary like this, but I was following the lead of the other declarations there. It's normally possible to use the parameter name in the typemap, but the parameters aren't named consistently (and sometimes there's param_list1, param_list2, etc.). I'm attaching a new patch that introduces a QofQueryParamList typedef. I hope I caught all GSLists that should be QofQueryParamList (I've also looked inside functions for consistency). Btw, it seems that quite some places in libqof are using GSList (which is a string list, right?) to store all kinds of stuff that is not a string. I think they should be GList instead, though GSList is probably just a typedef for GList). Also, it seems that the typemap for these param lists (with conversion function gnc_query_scm2path) is very similar to the typemap for general string lists (with conversion function gnc_scm_to_gslist_string, though it seems this is only used for GSList *key_path). The only real difference I can see is that the former actually checks if it gets passed a list of strings, while the latter just assumes that. Also, it seems that the former is actually used by some other conversion functions, so perhaps that explains the extra checks? Anyway, perhaps these two functions could be unified and a single typemap could be provided for all GSList* values, but I'm not confident enough about the possible side-effects of such a change...).
> GSList (which is a string list, right?) Nope. GSList is a Singly Linked List, whereas GList is a doubly-linked list.
r19863, thanks. As for your comment#6 patch: Does Derek's reminder about the meaning of a GSList change your patch, or is it still valid?
It makes the patch even more valid, since it means that a general approach for the "string lists" that I imagined is not possible.
Comment on attachment 175060 [details] [review] 0001-Add-a-typemap-for-a-QofQueryParamList-typedef.patch r19890, thanks a lot!
GnuCash bug tracking has moved to a new Bugzilla host. This bug has been copied to https://bugs.gnucash.org/show_bug.cgi?id=635469. Please update any external references or bookmarks.