GdPDU Export by jhs-s · Pull Request #2116 · Gnucash/gnucash

jhs-s · 2025-08-04T20:11:21Z

German tax authorities expect an standardized export of the electronic accounting in a Format called GoBD or GdPDU. This is essentially an CSV Format which can contain multiple files and an index.xml file describing those files. The format is rather flexible but it still expects that a booking is always on Record.

This branch adds an Option for exporting all bookings in a certain CSV-Format and takes care that "multiple bookings" are exported as several single bookings. The standard CSV export cannot do that and the extended CSV export is not able to but a booking into a single line.

This involved a refactoring of the CSV-Export, creating a new class that takes care of the different options.

There might be open points as I could only test with my accounting data which might not be fully representative. It also cannot cope with multiple bookings that do not have a single origin account. In this case it will abort. However, normal invoice bookings with VAT shouldn't have any issues.

There is a (german) description here: https://wiki.gnucash.org/wiki/De/howto

jralls · 2025-08-04T20:30:26Z

Merge branch 'master' into gdpdu-export

Don't do that, it messes up history. Always rebase your feature branch on the main branch (in this case stable).

contrib: Add necessary files for full gdpdu-export

I think these files would fit better in data.

jralls

Out of time for now, and I got through only one commit. More later.

jralls · 2025-08-04T20:51:00Z

gnucash/import-export/csv-exp/csv-export-helpers.cpp

-bool
-gnc_csv_add_line (std::ostream& ss, const StringVec& str_vec,
-                  bool use_quotes, const char* sep)
+bool gnc_csv_add_line(std::ostream &ss, const StringVec &str_vec,
+                      bool use_quotes, const char *sep)


Please follow the Coding Standard: In a definition the function name should start on a new line to make it easy to grep for, e.g. grep -rl ^gnc_csv_add_line gnucash/import-export. Declarations should have the return type, storage class, etc. on the same line as the function name.

jralls · 2025-08-04T20:59:58Z

gnucash/import-export/csv-exp/csv-export-helpers.cpp

+    }
+    catch (const std::exception &e)
+    {
+        std::cerr << e.what() << '\n';


GnuCash is primarily a GUI application that runs without a console so output to std::cout and std::cerr are lost; on Windows they're lost even with a console. We have a logging library that writes to a file instead with handy macros PERR, PWARN, PINFO, and PDEBUG. In this case PWARN seems appropriate: `PWARN("Exception %s", e.what(). The macro will add a timestamp and the name of the function.

jralls · 2025-08-04T21:01:56Z

gnucash/import-export/csv-exp/csv-transaction-export-line.cpp

+ * csv-transactions-export-line.cpp -- Convert Transaction to line    *
+ *                                                                  *
+ * Copyright (C) 2025 Johannes Triegel                                *
+ *                                                                  *


Please fix up the whitespace so the ending *s line up.

jralls · 2025-08-04T21:32:00Z

gnucash/import-export/csv-exp/csv-transaction-export-line.cpp

+                                                   bool simple,
+                                                   std::ofstream &ss) : m_split(split),
+                                                                        m_transaction(transaction),
+                                                                        m_separator(separator),
+                                                                        m_use_quotes(use_quotes),
+                                                                        m_simple(simple),
+                                                                        m_ss(ss),
+                                                                        m_is_debit_split(false),
+                                                                        m_is_credit_split(false),
+                                                                        m_base_split(NULL),
+                                                                        m_base_split_account(NULL)


Please start the initializers on a new line and put several on a line to cut down on the empty real estate.

jralls · 2025-08-04T21:38:05Z

gnucash/import-export/csv-exp/csv-transaction-export-line.cpp

+        }
+        if (debit > 1 && credit > 1)
+        {
+            std::cerr << "Multi split booking that needs to be booked on an intermediate account, no implemented!" << std::endl;


Use PINFO instead of std::cerr.

Also a typo: "not implemented" instead of "no implemented".

jralls · 2025-08-04T21:58:42Z

gnucash/import-export/csv-exp/csv-transactions-export.cpp


    /* Run the query */
-    for (GList *splits = qof_query_run (info->query); !info->failed && splits;
+    for (GList *splits = qof_query_run(info->query); /*!info->failed && */ splits;


Don't comment out code, remove it.

This should be a separate commit with the commit message explaining why you don't want to abort the loop on a print_csv failure.

jralls · 2025-08-04T21:59:37Z

gnucash/import-export/csv-exp/csv-transactions-export.cpp

+        if (!trans_set.emplace(trans).second)
+        {
            continue;
+        }


Don't add braces around single-line conditional or loop bodies..

jralls · 2025-08-04T22:01:35Z

gnucash/import-export/csv-exp/csv-transactions-export.cpp

+        CsvTransactionExportLine line(split, trans, info->separator_str, info->use_quotes, info->simple_layout, ss);
+        info->failed = !line.print_csv();

+#if 0


#if 0 is the same as comments: Just remove the code.

jralls

I don't see how CsvTransactionExportLine contributes anything except unnecessary overhead, and a major refactoring like this demands that you start by adding tests that prove the behavior of the existing code, then do the refactor with the tests still passing.

A PR shouldn't have "fix" commits in it: All of that should be cleaned up before submission.

But even if this were well designed and coded--and it isn't--I'd be hesitant to merge it because it's of use in only in Germany and we have no German core developers to maintain it.

jralls · 2025-08-05T18:12:24Z

gnucash/import-export/csv-exp/csv-transaction-export-line.hpp

+
+    bool print_csv();
+
+protected:


protected is to allow access to derived classes. CsvTransactionExportLine isn't an abstract class--it doesn't even have any virtual members--so there should be no protected members, only private and public. These are all getters so there's no good reason for them to be private.

jralls · 2025-08-05T18:25:13Z

gnucash/import-export/csv-exp/csv-transaction-export-line.hpp

+    const char *m_separator;
+    bool m_use_quotes;
+    bool m_simple;
+    std::ofstream &m_ss;


These should be passed as parameters to the relevant function, not held as object state.

jralls · 2025-08-05T18:26:03Z

gnucash/import-export/csv-exp/csv-transaction-export-line.cpp

+                                                                        m_base_split(NULL),
+                                                                        m_base_split_account(NULL)


This is C++. NULL is spelled nullptr.

jralls · 2025-08-05T18:28:16Z

gnucash/import-export/csv-exp/csv-transaction-export-line.cpp

+        }
+
+        // Find the base_split
+        Split *m_base_split = NULL;


You already did that in the initializers.

jralls · 2025-08-05T18:45:16Z

gnucash/import-export/csv-exp/csv-transactions-export.cpp

-}
-
-using TransSet = std::unordered_set<Transaction*>;
+using TransSet = std::unordered_set<Transaction *>;


That's innapropriate whitespace. Transaction* is a C++ type. It has been C++ convention basically forever that type names are kept together, though because C++'s backward compatibility with C allows a line like
Transaction* foo, bar;
would make foo a Transaction* and bar a Transaction it's necessary to either write
Transaction *foo, *bar; with the pointer indicator stuck to the variables for consistency or

Transaction *foo; Transaction *bar;

Stroustroup (Bjarne Stroustoup, The C++ Programming Language Fourth Edition, Addison Wesley, 2013, p 155) writes of the first way "Such declarations with multiple names and nontrivial declarators make a program harder to read and should be avoided." While GnuCash has a C heritage and most of the C++ files are largely renamed C code containing C conventions that's no reason to use C coding conventions in new C++ code.

jralls · 2025-08-05T19:43:38Z

gnucash/import-export/csv-exp/csv-transaction-export-line.hpp

    bool m_simple;
+    bool m_gdpdu;


Another case where an enum would be better than an accumulation of bools plus a hidden default when both are false.

jralls · 2025-08-05T19:55:52Z

gnucash/import-export/csv-exp/csv-transactions-export.cpp

+        /* Translators: The following symbols will build the header
+           line of exported CSV files for german GdPDU-Export: */


That doesn't work: Each translatable string is presented separately on Weblate; translator comments are presented only when the very next line is a translatable string and this one would be applied to only "Date". Also all of these strings are already in use without context. On the one hand that's good because you don't have to worry about the translations. Just check de.po to make sure that the msgstrs for each one are correct for this case.

jralls · 2025-08-05T19:59:47Z

gnucash/import-export/csv-exp/csv-transaction-export-line.cpp

+    auto type = xaccAccountGetType(account);
+    bool pos = gnc_numeric_positive_p(amount);
+
+    /* It is possible that other types of accounts need to be taken into account here */


This looks like a FIXME: GnuCash's account types aren't tested hierarchically so even though ACCT_TYPE_INCOME and ACCT_TYPE_EXPENSE are Equity they won't set exchange true.

A comment explaining why the presence of an Equity account justifies switching the order of the account numbers in a single transaction would be useful.

jralls · 2025-08-05T20:06:53Z

gnucash/import-export/csv-exp/csv-transaction-export-line.cpp

-StringVec
-CsvTransactionExportLine::make_simple_trans_split_line(Split *split)
-{
-    auto t_void{xaccTransGetVoidStatus(m_transaction)};
-    gnc_numeric amount = xaccSplitGetAmount(split);
-    bool pos = gnc_numeric_positive_p(amount);
-    return {
-        get_date(m_transaction),
-        pos ? xaccAccountGetName(m_base_split_account) : get_account_name(split, true),
-        get_number(m_transaction),
-        get_description(m_transaction),
-        pos ? get_account_name(split, true) : xaccAccountGetName(m_base_split_account),
-        get_reconcile(split),
-        get_amount(split, t_void, true),
-        get_amount(split, t_void, false),
-        get_value(split, t_void, true),
-        get_value(split, t_void, false),
-        get_rate(split, t_void)};
-}
-


This should be removed from the second commit instead of adding it there and throwing it away later.

jralls · 2025-08-05T20:11:26Z

gnucash/import-export/csv-exp/csv-transaction-export-line.cpp

 // Amount with Symbol or not
 std::string
-CsvTransactionExportLine::get_amount(Split *split, bool t_void, bool symbol)
+CsvTransactionExportLine::get_amount(Split *split, bool t_void, bool symbol, bool positive)


Multiple boolean function args is poor design, it's too easy to screw up the order.

jhs-s · 2025-10-13T17:50:26Z

Thanks for all your comments and sorry for not replying - I was assuming that github would somehow send me Mails on comments.

I will try to resolve the issues...

jhs-s · 2025-10-13T19:18:13Z

Merge branch 'master' into gdpdu-export

Don't do that, it messes up history. Always rebase your feature branch on the main branch (in this case stable).

Is there a clever way to do that without creating a new pull request. I guess I cannot change the existing history of the branch.

contrib: Add necessary files for full gdpdu-export

I think these files would fit better in data.

ok, no problem.

jralls · 2025-10-13T19:48:12Z

Is there a clever way to do that without creating a new pull request. I guess I cannot change the existing history of the branch.

You can change the branch history as much as you want back to the branch point, then force-push it (git push -f github gdpdu-export where github stands in for whatever you call this remote). That will update the PR, no new PR required.

jhs-s added 9 commits July 22, 2025 17:09

csv-export: Better error handling

4b9ee03

csv-export: Export split transaction in simple export

0d311a2

csv-export: Add GdPDU-Format to assistant

e477a0d

csv-export: Integrate non-simple export

207571d

csv-export: Fix gdpdu export after separation of simple export

a5229fb

csv-export: Fix GdPDU export sign

ec46dc7

csv-export: Make GdPdU-Export indepedant from simple-export

96e5b26

Merge branch 'master' into gdpdu-export

819a5e2

contrib: Add necessary files for full gdpdu-export

13bf618

jralls requested changes Aug 4, 2025

View reviewed changes

jralls requested changes Aug 5, 2025

View reviewed changes

jhs-s marked this pull request as draft October 13, 2025 19:23

		/* Translators: The following symbols will build the header
		line of exported CSV files for german GdPDU-Export: */

Conversation

jhs-s commented Aug 4, 2025

Uh oh!

jralls commented Aug 4, 2025

Uh oh!

jralls left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jralls left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhs-s commented Oct 13, 2025

Uh oh!

jhs-s commented Oct 13, 2025

Uh oh!

jralls commented Oct 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants