Skip to content

Commit

Permalink
Rework default non-currency commodity namespace.
Browse files Browse the repository at this point in the history 
Separate the "All noncurrency" convenience category in the commodity
selector and the default non-commodity namespace proposed by the QIF
importer because they have different functions.

Also remove the namespace guessing code from qif-dialog because with
only one default non-currency namespace there's nothing to guess.
  • Loading branch information
@jralls
jralls committed Mar 25, 2022
1 parent 9cde35a commit 109efe6
Show file tree
Hide file tree
Showing 7 changed files with 24 additions and 30 deletions.
3 changes: 0 additions & 3 deletions bindings/guile/engine.scm
View file
Expand Up @@ -37,9 +37,6 @@
(export account-full-name<?)
(export accounts-get-children-depth)

(define-public GNC_COMMODITY_NS_CURRENCY "CURRENCY")
(define-public GNC_COMMODITY_NS_NONCURRENCY (gettext "ALL NON-CURRENCY"))

(define (gnc-pricedb-lookup-latest-before-t64 . args)
(issue-deprecation-warning "gnc-pricedb-lookup-latest-before-t64 has been renamed to gnc-pricedb-lookup-nearest-before-t64")
(apply gnc-pricedb-lookup-nearest-before-t64 args))
Expand Down
2 changes: 1 addition & 1 deletion gnucash/gnome-utils/dialog-commodity.c
View file
Expand Up @@ -617,7 +617,7 @@ gnc_ui_update_namespace_picker (GtkWidget *cbwe,
{
gtk_list_store_append(GTK_LIST_STORE(model), &iter);
gtk_list_store_set (GTK_LIST_STORE(model), &iter, 0,
GNC_COMMODITY_NS_NONCURRENCY, -1);
GNC_COMMODITY_NS_NONISO_GUI, -1);
}

/* add all others to the combobox */
Expand Down
34 changes: 15 additions & 19 deletions gnucash/import-export/qif-imp/qif-dialog-utils.scm
View file
Expand Up @@ -724,6 +724,11 @@
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
(define (qif-dialog:default-namespace qif-symbol qif-type prefs)

(define (currency_ns? ns)
(or (string=? (GNC-COMMODITY-NS-CURRENCY) ns)
(string=? (GNC-COMMODITY-NS-LEGACY) ns)
(string=? (GNC-COMMODITY-NS-ISO4217) ns)))

This comment has been minimized.

Sign in to view

Copy link
@christopherlam

christopherlam Mar 27, 2022

Contributor

guile compiler complains GNC-COMMODITY-NS-ISO4217 is undefined.

This comment has been minimized.

Sign in to view

Copy link
@jralls

jralls Mar 27, 2022

Author Member

Right, because it should be (GNC-COMMODITY-NS-ISO). It doesn't break things with Guile 2; does it with Guile 3?

This comment has been minimized.

Sign in to view

Copy link
@christopherlam

christopherlam Mar 28, 2022

Contributor

The compilation logs a warning but doesn't fail. Reaching this unbound variable will definitely crash. Code that doesn't reach this variable will continue.

scheme@(guile-user)> (define t #t)
scheme@(guile-user)> (define (x)
...  (if t 'a b))
;;; <stdin>:3:1: warning: possibly unbound variable `b'
scheme@(guile-user)> (x)
$1 = a
scheme@(guile-user)> (define t #f)
scheme@(guile-user)> (x)
ice-9/boot-9.scm:1669:16: In procedure raise-exception:
Unbound variable: b

Entering a new prompt.  Type `,bt' for a backtrace or `,q' to continue.
scheme@(guile-user) [1]> 

scheme@(guile-user)>

This comment has been minimized.

Sign in to view

Copy link
@jralls

jralls Mar 28, 2022

Author Member

Fixed in git.

Not very good that the compiler only warns for a crasher, but I think the path is pretty unlikely, so I don't see a need for a snap release.

This comment has been minimized.

Sign in to view

Copy link
@christopherlam

christopherlam Mar 28, 2022

Contributor

I'm not by any means a scheme expert, but I suspect this is allowed because runtime code is allowed to do such things as (load "a-module.scm") which will then load variables. We can always call (defined? 'sym) before trying to access sym in code.

This comment has been minimized.

Sign in to view

Copy link
@christopherlam

christopherlam Mar 28, 2022

Contributor

FWIW I didn't run this code path to find these errors; guild did log them (I think when compiling .scm to .go).

;; Guess a namespace based on the symbol alone.
(define (guess-by-symbol s)
(if (string? s)
Expand All @@ -736,28 +741,19 @@
;; compatible with the QIF type?
(and (string=? s (caddr elt))
(not (and (string? qif-type)
(string=? GNC_COMMODITY_NS_NONCURRENCY
(cadr elt))
(not (currency_ns? (cadr elt))

This comment has been minimized.

Sign in to view

Copy link
@christopherlam

christopherlam Mar 27, 2022

Contributor

This (not ...) lacks a closing-parenthesis which has likely gone to line 749.

This comment has been minimized.

Sign in to view

Copy link
@jralls

jralls Mar 27, 2022

Author Member

No, its closing paren is correctly on line 749: (currency_ns? (cadr elt)) is a drop-in replacement for (string=? GNC_COMMODITY_NS_NONCURRENCY (cadr elt)); the logic is if not {qif-type is a string and namespace isn't a currency and qif-type is one of the listed types} ...

This comment has been minimized.

Sign in to view

Copy link
@christopherlam

christopherlam Mar 28, 2022

Contributor

But (not ...) always accepts only 1 argument. This code is currently written as (not Currency-test Or-string-tests).

This comment has been minimized.

Sign in to view

Copy link
@jralls

jralls Mar 28, 2022

Author Member

Got it. Fixed in git.
(or (string-ci=? qif-type "stock")
(string-ci=? qif-type "etf"))))))
(string-ci=? qif-type "etf")
(string-ci=? qif-type "mutual fund")
(string-ci=? qif-type "index")
)))))
prefs)
#f)))
#f))))
;; If a preferences match was found, use its namespace.
(if pref-match (cadr pref-match))
;; There's no symbol. Default to a fund.
GNC_COMMODITY_NS_NONCURRENCY)))

;; Was a QIF type given?
(if (string? qif-type)
;; Yes. We might be able to definitely determine the namespace.
(if (or
(string-ci=? qif-type "mutual fund")
(string-ci=? qif-type "index"))
GNC_COMMODITY_NS_NONCURRENCY
(guess-by-symbol qif-symbol)))

;; No QIF type was given, so guess a
;; default namespace by symbol alone.
(if pref-match (cadr pref-match)))
;; There's no symbol. Use the built-in default.
(GNC-COMMODITY-NS-NONCURRENCY)))

(guess-by-symbol qif-symbol))


Expand Down
2 changes: 1 addition & 1 deletion gnucash/import-export/qif-imp/qif-to-gnc.scm
View file
Expand Up @@ -270,7 +270,7 @@
(default-currency
(gnc-commodity-table-find-full
(gnc-commodity-table-get-table (gnc-get-current-book))
GNC_COMMODITY_NS_CURRENCY default-currency-name))
(GNC-COMMODITY-NS-CURRENCY) default-currency-name))
(sorted-accounts-list '())
(markable-xtns '())
(sorted-qif-files-list (sort qif-files-list
Expand Down
6 changes: 3 additions & 3 deletions libgnucash/app-utils/options.scm
View file
Expand Up @@ -435,7 +435,7 @@ the option '~a'."))
(if (string? currency)
(gnc-commodity-table-lookup
(gnc-commodity-table-get-table (gnc-get-current-book))
GNC_COMMODITY_NS_CURRENCY currency)
(GNC-COMMODITY-NS-CURRENCY) currency)
currency))

(let* ((value (currency->scm default-value))
Expand Down Expand Up @@ -561,7 +561,7 @@ the option '~a'."))
(define (commodity->scm commodity)
(if (string? commodity)
(list 'commodity-scm
GNC_COMMODITY_NS_CURRENCY
(GNC-COMMODITY-NS-CURRENCY)
commodity)
(list 'commodity-scm
(gnc-commodity-get-namespace commodity)
Expand Down Expand Up @@ -1560,7 +1560,7 @@ the option '~a'."))
(if (string? currency-string)
(gnc-commodity-table-lookup
(gnc-commodity-table-get-table (gnc-get-current-book))
GNC_COMMODITY_NS_CURRENCY currency-string)
(GNC-COMMODITY-NS-CURRENCY) currency-string)
#f))

(define (currency? val)
Expand Down
3 changes: 1 addition & 2 deletions libgnucash/engine/gnc-commodity.c
View file
Expand Up @@ -2212,7 +2212,7 @@ gnc_commodity_table_get_commodities(const gnc_commodity_table * table,

if (!table)
return NULL;
if (g_strcmp0(name_space, GNC_COMMODITY_NS_NONCURRENCY) == 0)
if (g_strcmp0(name_space, GNC_COMMODITY_NS_NONISO_GUI) == 0)
return commodity_table_get_all_noncurrency_commodities(table);
ns = gnc_commodity_table_find_namespace(table, name_space);
if (!ns)
Expand Down Expand Up @@ -2510,7 +2510,6 @@ gnc_commodity_table_add_default_data(gnc_commodity_table *table, QofBook *book)
gnc_commodity* c;

ENTER ("table=%p", table);
gnc_commodity_table_add_namespace(table, GNC_COMMODITY_NS_NONCURRENCY, book);
gnc_commodity_table_add_namespace(table, GNC_COMMODITY_NS_TEMPLATE, book);
c = gnc_commodity_new(book, "template", GNC_COMMODITY_NS_TEMPLATE, "template", "template", 1);
gnc_commodity_table_insert(table, c);
Expand Down
4 changes: 3 additions & 1 deletion libgnucash/engine/gnc-commodity.h
View file
Expand Up @@ -109,7 +109,9 @@ GType gnc_commodity_namespace_get_type(void);
/* The ISO define is deprecated in favor of CURRENCY */
#define GNC_COMMODITY_NS_ISO "ISO4217"
#define GNC_COMMODITY_NS_CURRENCY "CURRENCY"
#define GNC_COMMODITY_NS_NONCURRENCY NC_("Commodity Type", "All non-currency")
#define GNC_COMMODITY_NS_NONCURRENCY "NONCURRENCY"

#define GNC_COMMODITY_NS_NONISO_GUI NC_("Commodity Type", "All non-currency")
#define GNC_COMMODITY_NS_ISO_GUI NC_("Commodity Type", "Currencies")

/** Max fraction is 10^9 because 10^10 would require changing it to an
Expand Down

2 comments on commit 109efe6

@christopherlam
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't object to changing the var names to use underscore. If we care about assisting hidden report writers then we could add something like (untested):

(define-syntax-rule GNC_COMMODITY_NS_CURRENCY 
  (begin
   (issue-deprecation-warning "GNC_COMMODITY_NS_CURRENCY is no longer in use. ")
   "CURRENCY"))

@jralls
Copy link
Member Author

@jralls jralls commented on 109efe6 Apr 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be a better message to say use (GNC-COMMODITY-NS-CURRENCY) instead?

Please sign in to comment.