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

hledger bs begin date regression #531

Open
markokocic opened this Issue Mar 30, 2017 · 16 comments

Comments

Projects
None yet
3 participants
@markokocic
Collaborator

markokocic commented Mar 30, 2017

When using hledger bs in combination with the date restriction (e.g hledger bs date:from2017/03/28 display of results doesn't work as expected.

Expected (previous):
It was displaying bs values by taking into account only transactions that match date filter.

Actual:
It displays only account that match the filter, but instead of showing only totals of transactions matching the filter, it displays totals of all transactions. Seems like filter is applied only on accounts, and not on values.

Note:
Up until recently hledger bs worked as expected. Both accounts and values are filtered according to filter.
hledger inc works as expected.


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 Mar 30, 2017

Owner

@markokocic would you mind pasting a small example of bad and good behaviour ?

Owner

simonmichael commented Mar 30, 2017

@markokocic would you mind pasting a small example of bad and good behaviour ?

@markokocic

This comment has been minimized.

Show comment
Hide comment
@markokocic

markokocic Mar 30, 2017

Collaborator

Take the following example

marko@sky:~$ cat test.journal
2017/01/01 Test
        expenses:exp1  200
        assets:ass1

2017/01/01 Test
        expenses:exp2  300
        assets:ass1

2017/03/01 Test
        expenses:exp1  100
        assets:ass1

marko@sky:~$ hledger bs date:from2017/02/01 -f test.journal
Balance Sheet

Assets:
                -600  assets
                -600    ass1
--------------------
                -600

Liabilities:
--------------------
                   0

Total:
--------------------
                -600
marko@sky:~$

Since only the last transaction matches the filter criteria, I would expect that the result should look something like

Assets:
                -100  assets
                -100    ass1
--------------------
                -100

Liabilities:
--------------------
                   0

Total:
--------------------
                -100
Collaborator

markokocic commented Mar 30, 2017

Take the following example

marko@sky:~$ cat test.journal
2017/01/01 Test
        expenses:exp1  200
        assets:ass1

2017/01/01 Test
        expenses:exp2  300
        assets:ass1

2017/03/01 Test
        expenses:exp1  100
        assets:ass1

marko@sky:~$ hledger bs date:from2017/02/01 -f test.journal
Balance Sheet

Assets:
                -600  assets
                -600    ass1
--------------------
                -600

Liabilities:
--------------------
                   0

Total:
--------------------
                -600
marko@sky:~$

Since only the last transaction matches the filter criteria, I would expect that the result should look something like

Assets:
                -100  assets
                -100    ass1
--------------------
                -100

Liabilities:
--------------------
                   0

Total:
--------------------
                -100
@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Mar 30, 2017

Owner

I see. I asked @mstksg to make it ignore any begin date, because a real balance sheet doesn't have a begin date; it shows historical balances, ie from all time. That said, you can usebs --change to see that kind of report.

Owner

simonmichael commented Mar 30, 2017

I see. I asked @mstksg to make it ignore any begin date, because a real balance sheet doesn't have a begin date; it shows historical balances, ie from all time. That said, you can usebs --change to see that kind of report.

@mstksg

This comment has been minimized.

Show comment
Hide comment
@mstksg

mstksg Mar 30, 2017

Collaborator

Yes, I believe that it was discussed that in the domain of accounting, "balance sheet" is a well-defined and standard term that contains point-in-time balances; anything else wouldn't be an actual balance sheet by the accepted definition of the term. However, --change was indeed recently supported because I felt that the strict definition of "balance sheet" isn't the only useful way of displaying this.

Collaborator

mstksg commented Mar 30, 2017

Yes, I believe that it was discussed that in the domain of accounting, "balance sheet" is a well-defined and standard term that contains point-in-time balances; anything else wouldn't be an actual balance sheet by the accepted definition of the term. However, --change was indeed recently supported because I felt that the strict definition of "balance sheet" isn't the only useful way of displaying this.

@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Mar 30, 2017

Owner

More: --change was added to the generic balance command, for symmetry with --cumulative/--historical, and consistency with hledger-ui. And balancesheet has recently been acquiring more of balance's features, including this one. Arguably the traditional-accounting balancesheet/incomestatement/cashflow commands should not let you change the summation mode at all, but the flags came for free so we're just letting them be for now.

Owner

simonmichael commented Mar 30, 2017

More: --change was added to the generic balance command, for symmetry with --cumulative/--historical, and consistency with hledger-ui. And balancesheet has recently been acquiring more of balance's features, including this one. Arguably the traditional-accounting balancesheet/incomestatement/cashflow commands should not let you change the summation mode at all, but the flags came for free so we're just letting them be for now.

@markokocic

This comment has been minimized.

Show comment
Hide comment
@markokocic

markokocic Mar 31, 2017

Collaborator

Well, it's more of a matter of expectations. I understand filters as a mean of filtering input transactions, that could later be used for anything. And there, filtering by date has the same meaning as filtering by any other criteria. Why allow one, and not the other? Why allowing filter by account, and not by date?

I would expect that hledger bs date:from2017/02/01 has the same output as hledger print date:from2017/02/01 | hledger bs -f - And it was like that up until recently.

It would be more consistent to keep the old behaviour and use filters for filtering input set, and let all commands operate on that filtered input set.

Collaborator

markokocic commented Mar 31, 2017

Well, it's more of a matter of expectations. I understand filters as a mean of filtering input transactions, that could later be used for anything. And there, filtering by date has the same meaning as filtering by any other criteria. Why allow one, and not the other? Why allowing filter by account, and not by date?

I would expect that hledger bs date:from2017/02/01 has the same output as hledger print date:from2017/02/01 | hledger bs -f - And it was like that up until recently.

It would be more consistent to keep the old behaviour and use filters for filtering input set, and let all commands operate on that filtered input set.

@markokocic

This comment has been minimized.

Show comment
Hide comment
@markokocic

markokocic Mar 31, 2017

Collaborator

Btw, even the described scenario doesn't work. If date:from filter is going to be ignored in bs command, I would expect that the following two commands give the same output, but they don't:

marko@sky:~$ cat test.journal
2017/01/01 Test
        expenses:exp1  200
        assets:ass1

2017/01/01 Test
        expenses:exp2  300
        assets:ass1

2017/01/01 Test
        expenses:exp1  150
        assets:ass2

2017/03/01 Test
        expenses:exp1  100
        assets:ass1

marko@sky:~$ hledger bs -f test.journal
Balance Sheet

Assets:
                -750  assets
                -600    ass1
                -150    ass2
--------------------
                -750

Liabilities:
--------------------
                   0

Total:
--------------------
                -750
marko@sky:~$ hledger bs date:from2017/02/01 -f test.journal
Balance Sheet

Assets:
                -750  assets
                -600    ass1
--------------------
                -750

Liabilities:
--------------------
                   0

Total:
--------------------
                -750
marko@sky:~$

As reported in original message, hledger bs seems to display only account that match the date filter, but show all values.

Collaborator

markokocic commented Mar 31, 2017

Btw, even the described scenario doesn't work. If date:from filter is going to be ignored in bs command, I would expect that the following two commands give the same output, but they don't:

marko@sky:~$ cat test.journal
2017/01/01 Test
        expenses:exp1  200
        assets:ass1

2017/01/01 Test
        expenses:exp2  300
        assets:ass1

2017/01/01 Test
        expenses:exp1  150
        assets:ass2

2017/03/01 Test
        expenses:exp1  100
        assets:ass1

marko@sky:~$ hledger bs -f test.journal
Balance Sheet

Assets:
                -750  assets
                -600    ass1
                -150    ass2
--------------------
                -750

Liabilities:
--------------------
                   0

Total:
--------------------
                -750
marko@sky:~$ hledger bs date:from2017/02/01 -f test.journal
Balance Sheet

Assets:
                -750  assets
                -600    ass1
--------------------
                -750

Liabilities:
--------------------
                   0

Total:
--------------------
                -750
marko@sky:~$

As reported in original message, hledger bs seems to display only account that match the date filter, but show all values.

@mstksg

This comment has been minimized.

Show comment
Hide comment
@mstksg

mstksg Mar 31, 2017

Collaborator

It can be argued that the expectation of someone coming in from accounting would be that balancesheet reflected what they have always known "balance sheet" to refer to from their experience in the industry, where the term has an unambiguous meaning. The "transactions in window" behavior is useful, but it's not what 'balance sheet' is defined as in the professional settings where it is used. All we'd be doing is presenting something that's not a balance sheet, but calling it one.

It'd be sort of like making a programming language with a data type called "String" that actually represented Integers. Anyone coming from any other programming language would expect something like 'String' to represent what 'string' typically means in a programming setting... it's not that Ints aren't useful, but that it only causes confusion when you use a well-established name with a well-established meaning and implement it as something that is different from what its well-established meaning is.

The "transactions within interval" mode is not a balance sheet, by the widely accepted and used definition of balance sheet. Maybe the best route is to provide it, but under a different name, to prevent confusion?

Collaborator

mstksg commented Mar 31, 2017

It can be argued that the expectation of someone coming in from accounting would be that balancesheet reflected what they have always known "balance sheet" to refer to from their experience in the industry, where the term has an unambiguous meaning. The "transactions in window" behavior is useful, but it's not what 'balance sheet' is defined as in the professional settings where it is used. All we'd be doing is presenting something that's not a balance sheet, but calling it one.

It'd be sort of like making a programming language with a data type called "String" that actually represented Integers. Anyone coming from any other programming language would expect something like 'String' to represent what 'string' typically means in a programming setting... it's not that Ints aren't useful, but that it only causes confusion when you use a well-established name with a well-established meaning and implement it as something that is different from what its well-established meaning is.

The "transactions within interval" mode is not a balance sheet, by the widely accepted and used definition of balance sheet. Maybe the best route is to provide it, but under a different name, to prevent confusion?

@mstksg

This comment has been minimized.

Show comment
Hide comment
@mstksg

mstksg Mar 31, 2017

Collaborator

The main issue is that "balance sheet" isn't a concept that hledger made up out of the blue; it's actually a term and concept from accounting. So if hledger implements a "balance sheet" mode, it should implement it appropriately according to the accounting concept it is adopted from; otherwise, why give it the same name?

Collaborator

mstksg commented Mar 31, 2017

The main issue is that "balance sheet" isn't a concept that hledger made up out of the blue; it's actually a term and concept from accounting. So if hledger implements a "balance sheet" mode, it should implement it appropriately according to the accounting concept it is adopted from; otherwise, why give it the same name?

@markokocic

This comment has been minimized.

Show comment
Hide comment
@markokocic

markokocic Mar 31, 2017

Collaborator

@mstksg I agree with your point abut well defined term in accounting. But does that mean that no filtering at all should be allowed in bs command, since then it's not balance sheet anymore? I'm not an accountant so I can't judge on that.

If we keep "Transaction in window" as a concept, an accountant will probably never want to use date:from filter, since it doesn't make any sense to him, and others would still be able to use it as they see fit.

Executing hledger bs ass2 -f test.journal in previous example is doing exactly "transaction in window", and not a full balance sheet.

The only reason I am complaining here is that the old "transaction in window" behaviour was consistent and easy to remember in all commands, and that this "fix" of bs command is actually regression in behaviour.

If hledger is to ignore some filters in some commands, I'd rather see error message instead of silently ignoring it.

Collaborator

markokocic commented Mar 31, 2017

@mstksg I agree with your point abut well defined term in accounting. But does that mean that no filtering at all should be allowed in bs command, since then it's not balance sheet anymore? I'm not an accountant so I can't judge on that.

If we keep "Transaction in window" as a concept, an accountant will probably never want to use date:from filter, since it doesn't make any sense to him, and others would still be able to use it as they see fit.

Executing hledger bs ass2 -f test.journal in previous example is doing exactly "transaction in window", and not a full balance sheet.

The only reason I am complaining here is that the old "transaction in window" behaviour was consistent and easy to remember in all commands, and that this "fix" of bs command is actually regression in behaviour.

If hledger is to ignore some filters in some commands, I'd rather see error message instead of silently ignoring it.

@mstksg

This comment has been minimized.

Show comment
Hide comment
@mstksg

mstksg Mar 31, 2017

Collaborator

I do feel your pain about a workflow being disrupted...it's definitely not an ideal thing :/ But, you can recover transaction-in-window and take the filter into account if you pass along the --change flag, which switches from "point in time" mode to "changes in the interval" mode. So if you use a filter, you'd have to use --change. I'm not sure if this is the "best" compromise, because it does break existing workflows :(

(re: the odd behavior in your last post, it's actually the same behavior you get with balance ^assets ^liabilities. Unless -E is given, --historical also hides accounts with no activity during the interval, even if the account doesn't end at $0. This behavior did surprise me at first. I don't know for sure, but I don't think it's an intentional design decision. It's "documented" for multi-column balance reports, but not for normal single-column balance reports. @simonmichael, do you think this is the best behavior here?)

Collaborator

mstksg commented Mar 31, 2017

I do feel your pain about a workflow being disrupted...it's definitely not an ideal thing :/ But, you can recover transaction-in-window and take the filter into account if you pass along the --change flag, which switches from "point in time" mode to "changes in the interval" mode. So if you use a filter, you'd have to use --change. I'm not sure if this is the "best" compromise, because it does break existing workflows :(

(re: the odd behavior in your last post, it's actually the same behavior you get with balance ^assets ^liabilities. Unless -E is given, --historical also hides accounts with no activity during the interval, even if the account doesn't end at $0. This behavior did surprise me at first. I don't know for sure, but I don't think it's an intentional design decision. It's "documented" for multi-column balance reports, but not for normal single-column balance reports. @simonmichael, do you think this is the best behavior here?)

@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Mar 31, 2017

Owner
Owner

simonmichael commented Mar 31, 2017

@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Mar 31, 2017

Owner

Context: this relates to #518 (#525, #527, #533...)

Owner

simonmichael commented Mar 31, 2017

Context: this relates to #518 (#525, #527, #533...)

@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Mar 31, 2017

Owner

More: actually I think bs has always ignored a begin date specified with -b (tested back to 0.27). But @markokocic's example above shows it was not ignoring a begin date specified with date:. At least the latest version is consistent in ignoring both!

Owner

simonmichael commented Mar 31, 2017

More: actually I think bs has always ignored a begin date specified with -b (tested back to 0.27). But @markokocic's example above shows it was not ignoring a begin date specified with date:. At least the latest version is consistent in ignoring both!

@simonmichael simonmichael changed the title from hledger bs regression to hledger bs begin date regression Mar 31, 2017

@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Mar 31, 2017

Owner

It's not even that "bs ignores begin date", it's "--historical ignores begin date, and --historical is the default mode for bs".

Owner

simonmichael commented Mar 31, 2017

It's not even that "bs ignores begin date", it's "--historical ignores begin date, and --historical is the default mode for bs".

@simonmichael

This comment has been minimized.

Show comment
Hide comment
@simonmichael

simonmichael Apr 1, 2017

Owner

For today's release I clarified the manual and usage for bs & co., mentioning historical mode and begin date. I left the --change/--cumulative/--historical flags in place for now. Let's do some testing and decide whether they should stay, be removed, raise warnings or what not.

Owner

simonmichael commented Apr 1, 2017

For today's release I clarified the manual and usage for bs & co., mentioning historical mode and begin date. I left the --change/--cumulative/--historical flags in place for now. Let's do some testing and decide whether they should stay, be removed, raise warnings or what not.

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