New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Accounts with '?' confound web/ui registers #498

Closed
chreekat opened this Issue Jan 28, 2017 · 6 comments

Comments

Projects
None yet
3 participants
@chreekat
Contributor

chreekat commented Jan 28, 2017

Accounts with question marks show up in balances, but their transactions are not listed in hledger ui -- --register nor hledger web.

1/1
    First?  $1
    Second

1/2
    Second  $1
    Third

1/3
    Third  $2
    First?

anim

I suspect the problem is the question mark is not escaped in accountNameToAccountRegex, but I'm still compiling the universe and haven't tried it out yet. Also not sure what your preferred solution would be in this case.

(I just noticed this while working on cleaning up some imported data... I don't intend to keep the question marks for long.)

@chreekat

This comment has been minimized.

Show comment
Hide comment
@chreekat

chreekat Jan 28, 2017

Contributor

Yeah, this "fixed" it:

diff --git a/hledger-lib/Hledger/Data/AccountName.hs b/hledger-lib/Hledger/Data/AccountName.hs
index d70fb50..42a4975 100644
--- a/hledger-lib/Hledger/Data/AccountName.hs
+++ b/hledger-lib/Hledger/Data/AccountName.hs
@@ -152,7 +152,10 @@ clipOrEllipsifyAccountName n = accountNameFromComponents . take n . accountNameC
 -- | Convert an account name to a regular expression matching it and its subaccounts.
 accountNameToAccountRegex :: AccountName -> Regexp
 accountNameToAccountRegex "" = ""
-accountNameToAccountRegex a = printf "^%s(:|$)" (T.unpack a)
+accountNameToAccountRegex a =
+    printf "^%s(:|$)"
+    $ regexReplaceBy "\\?" ("\\" <>)
+    $ (T.unpack a)
 
 -- | Convert an account name to a regular expression matching it but not its subaccounts.
 accountNameToAccountOnlyRegex :: AccountName -> Regexp
Contributor

chreekat commented Jan 28, 2017

Yeah, this "fixed" it:

diff --git a/hledger-lib/Hledger/Data/AccountName.hs b/hledger-lib/Hledger/Data/AccountName.hs
index d70fb50..42a4975 100644
--- a/hledger-lib/Hledger/Data/AccountName.hs
+++ b/hledger-lib/Hledger/Data/AccountName.hs
@@ -152,7 +152,10 @@ clipOrEllipsifyAccountName n = accountNameFromComponents . take n . accountNameC
 -- | Convert an account name to a regular expression matching it and its subaccounts.
 accountNameToAccountRegex :: AccountName -> Regexp
 accountNameToAccountRegex "" = ""
-accountNameToAccountRegex a = printf "^%s(:|$)" (T.unpack a)
+accountNameToAccountRegex a =
+    printf "^%s(:|$)"
+    $ regexReplaceBy "\\?" ("\\" <>)
+    $ (T.unpack a)
 
 -- | Convert an account name to a regular expression matching it but not its subaccounts.
 accountNameToAccountOnlyRegex :: AccountName -> Regexp
@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Jan 28, 2017

Owner

Nice catch! I would welcome a PR.

Owner

simonmichael commented Jan 28, 2017

Nice catch! I would welcome a PR.

@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Jan 28, 2017

Owner

PS I wonder if there's a more robust function in regex-tdfa, which powers our regexps.

Owner

simonmichael commented Jan 28, 2017

PS I wonder if there's a more robust function in regex-tdfa, which powers our regexps.

@chreekat

This comment has been minimized.

Show comment
Hide comment
@chreekat

chreekat Jan 28, 2017

Contributor

My thought exactly. :) I can wing a fix, but I'd be worried about missing corner cases. I looked briefly and didn't see anything good. PR incoming.

Contributor

chreekat commented Jan 28, 2017

My thought exactly. :) I can wing a fix, but I'd be worried about missing corner cases. I looked briefly and didn't see anything good. PR incoming.

chreekat added a commit to chreekat/hledger that referenced this issue Jan 28, 2017

chreekat added a commit to chreekat/hledger that referenced this issue Jan 28, 2017

@ony

This comment has been minimized.

Show comment
Hide comment
@ony

ony Jan 28, 2017

Collaborator

@simonmichael, wouldn't it be easier to use alternative ways for match? I.e.

data MatchTarget = TargetRegex Regex
                 | TargetCaseInsensitive String
                 | TargetExact String

Ideally regexp library should allow building Regex. There is Pattern and corresponding patternToRegex but it isn't much different from building string for parser.

Collaborator

ony commented Jan 28, 2017

@simonmichael, wouldn't it be easier to use alternative ways for match? I.e.

data MatchTarget = TargetRegex Regex
                 | TargetCaseInsensitive String
                 | TargetExact String

Ideally regexp library should allow building Regex. There is Pattern and corresponding patternToRegex but it isn't much different from building string for parser.

@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Jan 28, 2017

Owner

I don't know, probably more principled but is it worth the added complexity ? We are using regexps everywhere so I don't mind leaning on them. Alternate PR welcome.

Owner

simonmichael commented Jan 28, 2017

I don't know, probably more principled but is it worth the added complexity ? We are using regexps everywhere so I don't mind leaning on them. Alternate PR welcome.

simonmichael added a commit that referenced this issue Jan 30, 2017

mstksg added a commit to mstksg/hledger that referenced this issue Feb 3, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment