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

Use of digit groups separator requires presence of decimals #487

Closed
ony opened this Issue Jan 17, 2017 · 33 comments

Comments

Projects
None yet
3 participants
@ony
Collaborator

ony commented Jan 17, 2017

Consider example:

% COLUMNS=80 hledger reg -f -
commodity EUR
	format 1,000.00 EUR

commodity UAH
	format 1.000,00 UAH

2017/1/1
	expenses  1,000 EUR
	expenses  1.000 EUR
	expenses  1,000.00 EUR
	assets

2017/1/1
	expenses  1,000 UAH
	expenses  1.000 UAH
	expenses  1.000,00 UAH
	assets

This results in

2017/01/01                      expenses                  1.00 EUR      1.00 EUR
                                expenses                  1.00 EUR      2.00 EUR
                                expenses              1,000.00 EUR  1,002.00 EUR
                                assets               -1,002.00 EUR             0
2017/01/01                      expenses                  1,00 UAH      1,00 UAH
                                expenses                  1,00 UAH      2,00 UAH
                                expenses              1.000,00 UAH  1.002,00 UAH
                                assets               -1.002,00 UAH             0

Which is pretty unexpected.
It should be:

2017/01/01                      expenses              1,000.00 EUR  1,000.00 EUR
                                expenses                  1.00 EUR  1,001.00 EUR
                                expenses              1,000.00 EUR  2,001.00 EUR
                                assets               -2,001.00 EUR             0
2017/01/01                      expenses                  1,00 UAH      1,00 UAH
                                expenses              1.000,00 UAH  1.001,00 UAH
                                expenses              1.000,00 UAH  2.001,00 UAH
                                assets               -2.001,00 UAH             0

I have to be very careful when I write numbers with thousands and always put decimals separator to avoid treating my numbers smaller than they are in fact.

hledger should honor decimals and digit groups specification of commodity format.

I don't mind to use space as a separator, but this isn't allowed yet. See #330 .


Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Jan 17, 2017

Owner

It's not hard at all to confuse the style inferrer. Does specifying the style with a commodity directive help ?

Given a clear specification of what should happen above, we could improve style inference, maybe. I'm not sure it's the right approach though.

Owner

simonmichael commented Jan 17, 2017

It's not hard at all to confuse the style inferrer. Does specifying the style with a commodity directive help ?

Given a clear specification of what should happen above, we could improve style inference, maybe. I'm not sure it's the right approach though.

@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Jan 17, 2017

Owner

Oh you gave the expected result. I admit I haven't spent the time to understand what's happening here. Given the difficulty of inferring style correctly for all possible confusing inputs, I'm not even sure yet if this is a bug, or something we can't reasonably handle. Improvements to code or docs welcome.

Owner

simonmichael commented Jan 17, 2017

Oh you gave the expected result. I admit I haven't spent the time to understand what's happening here. Given the difficulty of inferring style correctly for all possible confusing inputs, I'm not even sure yet if this is a bug, or something we can't reasonably handle. Improvements to code or docs welcome.

@ony

This comment has been minimized.

Show comment
Hide comment
@ony

ony Jan 17, 2017

Collaborator

@simonmichael, example already includes commodity directive. That's the problem. I believe inferred styles have no effect on this. I guess this happens during parse of the amount. It simply parse both 30.2 and 30,2 equally without using information about commodity format.

P.S. Re inferred styles. Inferred amounts always use inferred styles and ignore declared ones.

Collaborator

ony commented Jan 17, 2017

@simonmichael, example already includes commodity directive. That's the problem. I believe inferred styles have no effect on this. I guess this happens during parse of the amount. It simply parse both 30.2 and 30,2 equally without using information about commodity format.

P.S. Re inferred styles. Inferred amounts always use inferred styles and ignore declared ones.

@simonmichael simonmichael added the A BUG label Jan 17, 2017

@ony

This comment has been minimized.

Show comment
Hide comment
@ony

ony Jan 17, 2017

Collaborator

At this moment I use hledger-check.hs to verify that number style is consistent (same precision and decimals separator).

Collaborator

ony commented Jan 17, 2017

At this moment I use hledger-check.hs to verify that number style is consistent (same precision and decimals separator).

@ony

This comment has been minimized.

Show comment
Hide comment
@ony

ony May 14, 2017

Collaborator

It is possible to solve it in an ugly way within current design. I.e. post-process entries without digit groups and that have decimal separator matching with commodity specification. But consider next journal:

commodity  $100,000.00
2016/1/1
    a  $-10.00
    b

commodity  $100.000,00
2017/1/1
    a  $-10,00
    b

I would expect in both cases it to be parsed as $-10 (i.e. last commodity directive have effect until next occurrence ).

Collaborator

ony commented May 14, 2017

It is possible to solve it in an ugly way within current design. I.e. post-process entries without digit groups and that have decimal separator matching with commodity specification. But consider next journal:

commodity  $100,000.00
2016/1/1
    a  $-10.00
    b

commodity  $100.000,00
2017/1/1
    a  $-10,00
    b

I would expect in both cases it to be parsed as $-10 (i.e. last commodity directive have effect until next occurrence ).

@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Aug 27, 2017

Owner

Note to self: check fix also resolved #545.

Owner

simonmichael commented Aug 27, 2017

Note to self: check fix also resolved #545.

@dashed

This comment has been minimized.

Show comment
Hide comment
@dashed

dashed Oct 28, 2017

This bit me when I tried hledger on files, originally procesed with ledger-cli v3, that have balances like 4,000 CAD (the comma is mis-interpreted as a decimal placeholder) :( And I have a lot of transactions of this form.

Is a fix being worked on?

I'd like to help, but I'm not proficient with haskell.

dashed commented Oct 28, 2017

This bit me when I tried hledger on files, originally procesed with ledger-cli v3, that have balances like 4,000 CAD (the comma is mis-interpreted as a decimal placeholder) :( And I have a lot of transactions of this form.

Is a fix being worked on?

I'd like to help, but I'm not proficient with haskell.

@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Oct 28, 2017

Owner

@dashed nobody is currently working on it; this issue is available, and so is haskell mentoring (in #hledger) :)

I think @ony's example above is more complicated than the norm. You can usually prevent this problem by simply ensuring the first amount of this commodity in the journal (or a commodity directive for this commodity) has both a digit group separator and a decimal point.

Owner

simonmichael commented Oct 28, 2017

@dashed nobody is currently working on it; this issue is available, and so is haskell mentoring (in #hledger) :)

I think @ony's example above is more complicated than the norm. You can usually prevent this problem by simply ensuring the first amount of this commodity in the journal (or a commodity directive for this commodity) has both a digit group separator and a decimal point.

@ony

This comment has been minimized.

Show comment
Hide comment
@ony

ony Oct 28, 2017

Collaborator

@simonmichael , commodity only contributes to how we render amount, as I know. This error happens during parsing and we don't have commodity directive there.
To get commodity influence how we parse we should have two pass parser (delay amount parsing) or context-aware parser (to get effect on rest of the file).

Collaborator

ony commented Oct 28, 2017

@simonmichael , commodity only contributes to how we render amount, as I know. This error happens during parsing and we don't have commodity directive there.
To get commodity influence how we parse we should have two pass parser (delay amount parsing) or context-aware parser (to get effect on rest of the file).

@dashed

This comment has been minimized.

Show comment
Hide comment
@dashed

dashed Oct 28, 2017

maybe another directive?

dashed commented Oct 28, 2017

maybe another directive?

@ony

This comment has been minimized.

Show comment
Hide comment
@ony

ony Oct 28, 2017

Collaborator

@dashed , I think we can re-use existing directive. It just a lot of changes associated with migrating from TextParser to JournalParser. You'll need to change all usages.
Besides that you'll need a way to to supply journal when amount from arguments being parsed (yes, that's another place this issue exposed).

Update: actually numberp have no information about symbol. So maybe we can just extend it.

Collaborator

ony commented Oct 28, 2017

@dashed , I think we can re-use existing directive. It just a lot of changes associated with migrating from TextParser to JournalParser. You'll need to change all usages.
Besides that you'll need a way to to supply journal when amount from arguments being parsed (yes, that's another place this issue exposed).

Update: actually numberp have no information about symbol. So maybe we can just extend it.

@ony ony self-assigned this Oct 28, 2017

@ony ony added the in progress label Oct 28, 2017

@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Oct 28, 2017

Owner

I see... we do already have a context-aware parser, but I think commodity directive should not be position sensitive, it should affect all data. Which sounds like a second pass. Making the parser even more complicated, ugh.

What if we disallow digit group separators in the journal ?

Owner

simonmichael commented Oct 28, 2017

I see... we do already have a context-aware parser, but I think commodity directive should not be position sensitive, it should affect all data. Which sounds like a second pass. Making the parser even more complicated, ugh.

What if we disallow digit group separators in the journal ?

@ony

This comment has been minimized.

Show comment
Hide comment
@ony

ony Oct 28, 2017

Collaborator

@simonmichael , err... I don't want to drop digits group. I'd rather then keep it as is.

I see... we do already have a context-aware parser, but I think commodity directive should not be position sensitive, it should affect all data.

I see benfit of allowing diferent files to have different format. So I'm not really into making a second pass. Though it a very simply hack to fix issue very fast.
If you don't want commodity to have ranged effect (until next such directive) we can use something like format as suggested by @dashed .And we probably should enforce to have only one commodity per currency within all files in this case.

Right now I'm working on introducing decimalHint to numberp as a dirty not too hacky solution.
(actually right now I'm working on tests to ensure that I don't break anything)

Collaborator

ony commented Oct 28, 2017

@simonmichael , err... I don't want to drop digits group. I'd rather then keep it as is.

I see... we do already have a context-aware parser, but I think commodity directive should not be position sensitive, it should affect all data.

I see benfit of allowing diferent files to have different format. So I'm not really into making a second pass. Though it a very simply hack to fix issue very fast.
If you don't want commodity to have ranged effect (until next such directive) we can use something like format as suggested by @dashed .And we probably should enforce to have only one commodity per currency within all files in this case.

Right now I'm working on introducing decimalHint to numberp as a dirty not too hacky solution.
(actually right now I'm working on tests to ensure that I don't break anything)

@dashed

This comment has been minimized.

Show comment
Hide comment
@dashed

dashed Oct 28, 2017

@ony unrelated; but, I was curious how you used hledger-check to check style consistency? (see: #487 (comment))

dashed commented Oct 28, 2017

@ony unrelated; but, I was curious how you used hledger-check to check style consistency? (see: #487 (comment))

@ony

This comment has been minimized.

Show comment
Hide comment
@ony

ony Oct 28, 2017

Collaborator

@dashed, my bad. I had to reference particular commit in my fork. I had hledger-check that check journal and someone introduced one to check for some assumptions. So now I have it as hledger-lint in my repo, but in needs to be fixed.

Collaborator

ony commented Oct 28, 2017

@dashed, my bad. I had to reference particular commit in my fork. I had hledger-check that check journal and someone introduced one to check for some assumptions. So now I have it as hledger-lint in my repo, but in needs to be fixed.

@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Oct 28, 2017

Owner

@ony I'm looking forward to your experiment. PS that was a good insight, the number of decimal places and the size of digit groups is needed only for output, but the identity of decimal point/digit group separator characters is needed for parsing (since we don't require amounts to always have both).

I do think we really need to fight against complexity, and that a position-sensitive commodity directive is unwelcome complexity for the user. We should check whether per-commodity amount styles are even necessary. Per-commodity precision is needed, I guess. Is allowing a different decimal point/digit group separator per commodity really needed, or even wise ? Should that be a journal-wide setting ? Just brainstorming.

Owner

simonmichael commented Oct 28, 2017

@ony I'm looking forward to your experiment. PS that was a good insight, the number of decimal places and the size of digit groups is needed only for output, but the identity of decimal point/digit group separator characters is needed for parsing (since we don't require amounts to always have both).

I do think we really need to fight against complexity, and that a position-sensitive commodity directive is unwelcome complexity for the user. We should check whether per-commodity amount styles are even necessary. Per-commodity precision is needed, I guess. Is allowing a different decimal point/digit group separator per commodity really needed, or even wise ? Should that be a journal-wide setting ? Just brainstorming.

@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Oct 28, 2017

Owner

Possibly slightly related: #633.

Owner

simonmichael commented Oct 28, 2017

Possibly slightly related: #633.

@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Oct 28, 2017

Owner

Second thoughts. Maybe it must be position-sensitive. Because you might be parsing multiple journals in a single command, from different source, which might have different styles. :/

We can easily get lost overengineering small details. Please let me know if you see any practical simplifying assumptions/constraints we could make.

Owner

simonmichael commented Oct 28, 2017

Second thoughts. Maybe it must be position-sensitive. Because you might be parsing multiple journals in a single command, from different source, which might have different styles. :/

We can easily get lost overengineering small details. Please let me know if you see any practical simplifying assumptions/constraints we could make.

@ony

This comment has been minimized.

Show comment
Hide comment
@ony

ony Oct 28, 2017

Collaborator

@simonmichael , that's other issue. I always have inconsistent rendering of the numbers and I gave up on it sometime when I wrote that hledger-check which expose both of these issues :)

@dashed, you can use hledger-lint to see difference in how number will be rendered and how it is written.

$ ./bin/hledger-lint -f sample.ledger
Commodity format mismatch for ₴
	Expect: ₴1,000.00
	Actual: ₴1,000,000
(inferred from journal)

Commodity format mismatch for ₴
	Expect: ₴1,000.00
	Actual: ₴1,000.0000
In context of
2017-01-01            liabilities      ₴0.0059  ;
Within transaction at JournalSourcePos "/home/nikolay/contrib/hledger/sample.ledger" (9,13)
2017/01/01 Issue with rendering
    liabilities    $-123 @ ₴22.9733
    expenses                ₴141.28
    assets                ₴2,684.43
    liabilities             ₴0.0059
$ cat sample.ledger 
commodity €1,000.00
commodity ₴1,000.00

2017/01/01 Issue with parsing
	liabilities  -₴1,000
	expenses  ₴500
	assets 

2017/01/01 Issue with rendering
	liabilities  -$123 @ ₴22.9733
	expenses  ₴141.28
	assets  ₴2,684.43
	liabilities
Collaborator

ony commented Oct 28, 2017

@simonmichael , that's other issue. I always have inconsistent rendering of the numbers and I gave up on it sometime when I wrote that hledger-check which expose both of these issues :)

@dashed, you can use hledger-lint to see difference in how number will be rendered and how it is written.

$ ./bin/hledger-lint -f sample.ledger
Commodity format mismatch for ₴
	Expect: ₴1,000.00
	Actual: ₴1,000,000
(inferred from journal)

Commodity format mismatch for ₴
	Expect: ₴1,000.00
	Actual: ₴1,000.0000
In context of
2017-01-01            liabilities      ₴0.0059  ;
Within transaction at JournalSourcePos "/home/nikolay/contrib/hledger/sample.ledger" (9,13)
2017/01/01 Issue with rendering
    liabilities    $-123 @ ₴22.9733
    expenses                ₴141.28
    assets                ₴2,684.43
    liabilities             ₴0.0059
$ cat sample.ledger 
commodity €1,000.00
commodity ₴1,000.00

2017/01/01 Issue with parsing
	liabilities  -₴1,000
	expenses  ₴500
	assets 

2017/01/01 Issue with rendering
	liabilities  -$123 @ ₴22.9733
	expenses  ₴141.28
	assets  ₴2,684.43
	liabilities
@dashed

This comment has been minimized.

Show comment
Hide comment
@dashed

dashed Oct 28, 2017

@ony oh sweet! I'll definitely check that script out 👍

dashed commented Oct 28, 2017

@ony oh sweet! I'll definitely check that script out 👍

@ony

This comment has been minimized.

Show comment
Hide comment
@ony

ony Oct 28, 2017

Collaborator

@dashed, I don't think it is useful after commit which will just fail parsing of your journal with exact location.

Summary of "hacking":

  • Though we have context-aware parsing we can't properly parse number in rightsymbolamountp because commodity information will go after number. So multi-pass parsing is something that we may need to consider. But that not necessary will end up with parseJournalNumbers :: JournalT (PostingT ... [String] {- number parts -}) -> JournalT (PostingT ... Amount). We still can limit impact only to rightsymbolamountp.
  • Since parsing of commodity directive itself is requires numberp it makes it really weird. I.e.
    commodity EUR 1,000 -- fail
    
    commodity EUR 0.00
    commodity EUR 1,000 -- succes
    
Collaborator

ony commented Oct 28, 2017

@dashed, I don't think it is useful after commit which will just fail parsing of your journal with exact location.

Summary of "hacking":

  • Though we have context-aware parsing we can't properly parse number in rightsymbolamountp because commodity information will go after number. So multi-pass parsing is something that we may need to consider. But that not necessary will end up with parseJournalNumbers :: JournalT (PostingT ... [String] {- number parts -}) -> JournalT (PostingT ... Amount). We still can limit impact only to rightsymbolamountp.
  • Since parsing of commodity directive itself is requires numberp it makes it really weird. I.e.
    commodity EUR 1,000 -- fail
    
    commodity EUR 0.00
    commodity EUR 1,000 -- succes
    

ony added a commit to ony/hledger that referenced this issue Oct 28, 2017

ony added a commit to ony/hledger that referenced this issue Oct 28, 2017

@ony

This comment has been minimized.

Show comment
Hide comment
@ony

ony Oct 28, 2017

Collaborator

@simonmichael, I guess you can consider #635 as a kinda fix for this. It might also simplify numbers parsing a bit.

We can easily get lost overengineering small details. Please let me know if you see any practical simplifying assumptions/constraints we could make.

One of the way to simplify is to say that dot ('.') is default separator and allow overriding it with comma (',') via some directive. But this might result in significant impact on users.

Collaborator

ony commented Oct 28, 2017

@simonmichael, I guess you can consider #635 as a kinda fix for this. It might also simplify numbers parsing a bit.

We can easily get lost overengineering small details. Please let me know if you see any practical simplifying assumptions/constraints we could make.

One of the way to simplify is to say that dot ('.') is default separator and allow overriding it with comma (',') via some directive. But this might result in significant impact on users.

@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Oct 29, 2017

Owner

@ony: thanks for the comments and patches, I am digesting/testing these as I find spare cycles.

Owner

simonmichael commented Oct 29, 2017

@ony: thanks for the comments and patches, I am digesting/testing these as I find spare cycles.

ony added a commit to ony/hledger that referenced this issue Nov 1, 2017

ony added a commit to ony/hledger that referenced this issue Nov 1, 2017

ony added a commit to ony/hledger that referenced this issue Nov 2, 2017

journal: use decimal sep hint for amount parser
Make use of commodity format directive as a hint for parsing amount.

Kinda resolves simonmichael#487
@dashed

This comment has been minimized.

Show comment
Hide comment
@dashed

dashed Nov 4, 2017

@ony I just got around to checking out hledger-lint script and I wasn't sure if I'm using it properly.

Contents of sample.ledger:

; sample.ledger
commodity €1,000.00
commodity ₴1,000.00

2017/01/01 Issue with parsing
    liabilities  -₴1,000
    expenses  ₴500
    assets

$ hledger -f sample.ledger bal
            ₴-499.00  assets
             ₴500.00  expenses
              ₴-1.00  liabilities
--------------------
                   0

$ ./hledger-lint -f sample.ledger
Commodity format mismatch for ₴
	Expect: ₴1,000.00
	Actual: ₴1000,000
(inferred from journal)


Contents of sample2.ledger:

; sample.ledger
commodity €1,000.00
commodity ₴1,000.00

2017/01/01 Issue with parsing
    liabilities  -₴1,000.00
    expenses  ₴500.00
    assets

$ hledger -f sample2.ledger bal
             ₴500.00  assets
             ₴500.00  expenses
          ₴-1,000.00  liabilities
--------------------
                   0

sample.ledger is not well-formed, but sample2.ledger is well-formed.

Shouldn't there be an error output for running ./hledger-lint -f sample.ledger?

dashed commented Nov 4, 2017

@ony I just got around to checking out hledger-lint script and I wasn't sure if I'm using it properly.

Contents of sample.ledger:

; sample.ledger
commodity €1,000.00
commodity ₴1,000.00

2017/01/01 Issue with parsing
    liabilities  -₴1,000
    expenses  ₴500
    assets

$ hledger -f sample.ledger bal
            ₴-499.00  assets
             ₴500.00  expenses
              ₴-1.00  liabilities
--------------------
                   0

$ ./hledger-lint -f sample.ledger
Commodity format mismatch for ₴
	Expect: ₴1,000.00
	Actual: ₴1000,000
(inferred from journal)


Contents of sample2.ledger:

; sample.ledger
commodity €1,000.00
commodity ₴1,000.00

2017/01/01 Issue with parsing
    liabilities  -₴1,000.00
    expenses  ₴500.00
    assets

$ hledger -f sample2.ledger bal
             ₴500.00  assets
             ₴500.00  expenses
          ₴-1,000.00  liabilities
--------------------
                   0

sample.ledger is not well-formed, but sample2.ledger is well-formed.

Shouldn't there be an error output for running ./hledger-lint -f sample.ledger?

@ony

This comment has been minimized.

Show comment
Hide comment
@ony

ony Nov 5, 2017

Collaborator

@dashed yes, that's an issue. It should report that style for -₴1,000 (integer 1 and fraction represented with precision of 3 digits separated by char ,) doesn't match with expected ₴1,000.00 (group of 3 digits separated by , and fraction with precision of 2 and separator .).

Yep. That was an issue. Mainly because that information were lost on the moment when hledger-lint receives journal. So input already had all the values normalized according to commodity directive. On the other hand you still get that error about "inferred" style which is different:

Commodity format mismatch for ₴
	Expect: ₴1,000.00
	Actual: ₴1000,000
(inferred from journal)

I've updated branch feature/journal-lint to fix that and get output:

Commodity format mismatch for ₴
	Expect: ₴1,000.00
	Actual: ₴1000,000
(inferred from journal)

Commodity format mismatch for ₴
	Expect: ₴1,000.00
	Actual: ₴1000,000
In context of
2017-01-01            liabilities      ₴-1,000  ;
Within transaction at JournalSourcePos "/home/nikolay/contrib/hledger/sample.ledger" (4,7)
2017/01/01 Issue with parsing
    liabilities         ₴-1,000
    expenses               ₴500
    assets

Note that print will preserve journal style for amounts with commit c31e028 . I guess discussion of that tool should be done in a separate thread to avoid confusion.

With #635 cases with misinterpreting groups separators as decimal part separator should be significantly reduced to only cases when we didn't specified commodity directive before affected transaction within a same file.

Collaborator

ony commented Nov 5, 2017

@dashed yes, that's an issue. It should report that style for -₴1,000 (integer 1 and fraction represented with precision of 3 digits separated by char ,) doesn't match with expected ₴1,000.00 (group of 3 digits separated by , and fraction with precision of 2 and separator .).

Yep. That was an issue. Mainly because that information were lost on the moment when hledger-lint receives journal. So input already had all the values normalized according to commodity directive. On the other hand you still get that error about "inferred" style which is different:

Commodity format mismatch for ₴
	Expect: ₴1,000.00
	Actual: ₴1000,000
(inferred from journal)

I've updated branch feature/journal-lint to fix that and get output:

Commodity format mismatch for ₴
	Expect: ₴1,000.00
	Actual: ₴1000,000
(inferred from journal)

Commodity format mismatch for ₴
	Expect: ₴1,000.00
	Actual: ₴1000,000
In context of
2017-01-01            liabilities      ₴-1,000  ;
Within transaction at JournalSourcePos "/home/nikolay/contrib/hledger/sample.ledger" (4,7)
2017/01/01 Issue with parsing
    liabilities         ₴-1,000
    expenses               ₴500
    assets

Note that print will preserve journal style for amounts with commit c31e028 . I guess discussion of that tool should be done in a separate thread to avoid confusion.

With #635 cases with misinterpreting groups separators as decimal part separator should be significantly reduced to only cases when we didn't specified commodity directive before affected transaction within a same file.

ony added a commit to ony/hledger that referenced this issue Nov 27, 2017

journal: use decimal sep hint for amount parser
Make use of commodity format directive as a hint for parsing amount.

Kinda resolves simonmichael#487

ony added a commit to ony/hledger that referenced this issue Nov 27, 2017

journal: use decimal sep hint for amount parser
Make use of commodity format directive as a hint for parsing amount.

Kinda resolves simonmichael#487
@dashed

This comment has been minimized.

Show comment
Hide comment
@dashed

dashed Nov 27, 2017

@simonmichael @ony just wanted to double check; according to the commit message of b7dbe04 :

Kinda resolves #487

dashed commented Nov 27, 2017

@simonmichael @ony just wanted to double check; according to the commit message of b7dbe04 :

Kinda resolves #487

@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Nov 28, 2017

Owner

@dashed: commodity or D (default commodity) directives, if any, are now considered when inferring the decimal point and digit group separator for that commodity. See latest http://hledger.org/manual.html#amounts (reload the page).

Let us know if this makes sense and/or would solve the problem you saw.

Owner

simonmichael commented Nov 28, 2017

@dashed: commodity or D (default commodity) directives, if any, are now considered when inferring the decimal point and digit group separator for that commodity. See latest http://hledger.org/manual.html#amounts (reload the page).

Let us know if this makes sense and/or would solve the problem you saw.

@dashed

This comment has been minimized.

Show comment
Hide comment
@dashed

dashed Dec 3, 2017

@simonmichael I just got to trying out master branch of hledger and it seems I need to insert the commodity directive on every ledger file. I'm unsure if this is intended.

For example:

; sample.ledger

commodity CAD
    format 1,000.00 CAD

commodity USD
    format 1,000.00 USD

!include ./sample_2.ledger

2017/01/01 works
    liabilities  -1,000 CAD
    expenses

and

; sample_2.ledger

2017/01/01 description
    liabilities  -1,000 CAD
    expenses

And output:

$ hledger -f sample.ledger bal
        1,001.00 CAD  expenses
       -1,001.00 CAD  liabilities
--------------------
                   0

dashed commented Dec 3, 2017

@simonmichael I just got to trying out master branch of hledger and it seems I need to insert the commodity directive on every ledger file. I'm unsure if this is intended.

For example:

; sample.ledger

commodity CAD
    format 1,000.00 CAD

commodity USD
    format 1,000.00 USD

!include ./sample_2.ledger

2017/01/01 works
    liabilities  -1,000 CAD
    expenses

and

; sample_2.ledger

2017/01/01 description
    liabilities  -1,000 CAD
    expenses

And output:

$ hledger -f sample.ledger bal
        1,001.00 CAD  expenses
       -1,001.00 CAD  liabilities
--------------------
                   0
@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Dec 3, 2017

Owner

Hmm, I see what you mean:

; a.j

commodity CAD
    format 1,000.00 CAD

2017/01/01  a
    (a)  -1,000 CAD

include b.j
; b.j

; this directive doesn't propagate from the parent file,
; seems we must repeat it here, or -1,000 CAD will be parsed as -1 CAD
commodity CAD
    format 1,000.00 CAD

2017/01/02
    (b)  -1,000 CAD
$ hledger -f a.j reg amt:1000
2017/01/01                     (a)                  -1,000.00 CAD  -1,000.00 CAD
2017/01/02                     (b)                  -1,000.00 CAD  -2,000.00 CAD

IIRC some (most? all?) directives affect only the current file. Actually I thought directives did generally affect child files, if not subsequent sibling files. Perhaps this can be easily fixed.

So yes, a work around is to repeat the commodity directive in each file (or include it in each from a common file).

Or, just make sure to write a decimal point in the first amount in each file, eg:

; a.j

2017/01/01
    (a)  -1,000.00 CAD

include b.j
; b.j

2017/01/02
    (b)  -1,000.00 CAD

Still, what should be done to minimise surprise ?

Owner

simonmichael commented Dec 3, 2017

Hmm, I see what you mean:

; a.j

commodity CAD
    format 1,000.00 CAD

2017/01/01  a
    (a)  -1,000 CAD

include b.j
; b.j

; this directive doesn't propagate from the parent file,
; seems we must repeat it here, or -1,000 CAD will be parsed as -1 CAD
commodity CAD
    format 1,000.00 CAD

2017/01/02
    (b)  -1,000 CAD
$ hledger -f a.j reg amt:1000
2017/01/01                     (a)                  -1,000.00 CAD  -1,000.00 CAD
2017/01/02                     (b)                  -1,000.00 CAD  -2,000.00 CAD

IIRC some (most? all?) directives affect only the current file. Actually I thought directives did generally affect child files, if not subsequent sibling files. Perhaps this can be easily fixed.

So yes, a work around is to repeat the commodity directive in each file (or include it in each from a common file).

Or, just make sure to write a decimal point in the first amount in each file, eg:

; a.j

2017/01/01
    (a)  -1,000.00 CAD

include b.j
; b.j

2017/01/02
    (b)  -1,000.00 CAD

Still, what should be done to minimise surprise ?

@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Dec 3, 2017

Owner

For context, one of the reasons directives don't affect sibling files is that it keeps -f options independent of each other, so you can freely add/remove/reorder files on the command line without fragility or complex interactions. I think generally we do expect directives to affect child files though.

Owner

simonmichael commented Dec 3, 2017

For context, one of the reasons directives don't affect sibling files is that it keeps -f options independent of each other, so you can freely add/remove/reorder files on the command line without fragility or complex interactions. I think generally we do expect directives to affect child files though.

@dashed

This comment has been minimized.

Show comment
Hide comment
@dashed

dashed Dec 4, 2017

Maybe a new commodity directive that does propagate to child files?

dashed commented Dec 4, 2017

Maybe a new commodity directive that does propagate to child files?

@ony

This comment has been minimized.

Show comment
Hide comment
@ony

ony Dec 4, 2017

Collaborator

@simonmichael , I'm not sure it is related to "scope rules" we have right now. Need to check how new parser created from old one I'm pretty sure that we don't clone collected commodities directives.
I agree with @dashed that having child files be affected will be more consistent with current behaviour of hledger than allowing include for something like head.journal in each file with transactions (that's what I tried also for aliases and other stuff).

Note that when I used ledger-clie I used to have file

include ledger-commodities.dat
include ledger-accounts.dat

include ledger-2008.dat
; ...

With hledger I had to inline first two files. And I guess I want to add new include directive that will have old behaviour. Like include-inplace or introduce special "journal" type.

Collaborator

ony commented Dec 4, 2017

@simonmichael , I'm not sure it is related to "scope rules" we have right now. Need to check how new parser created from old one I'm pretty sure that we don't clone collected commodities directives.
I agree with @dashed that having child files be affected will be more consistent with current behaviour of hledger than allowing include for something like head.journal in each file with transactions (that's what I tried also for aliases and other stuff).

Note that when I used ledger-clie I used to have file

include ledger-commodities.dat
include ledger-accounts.dat

include ledger-2008.dat
; ...

With hledger I had to inline first two files. And I guess I want to add new include directive that will have old behaviour. Like include-inplace or introduce special "journal" type.

@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Dec 4, 2017

Owner
Owner

simonmichael commented Dec 4, 2017

ony added a commit to ony/hledger that referenced this issue Dec 4, 2017

simonmichael added a commit that referenced this issue Dec 4, 2017

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