Skip to content
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

csv parser: mark guessed account as calculated #2086

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

madroach
Copy link

@madroach madroach commented Feb 1, 2022

This will change the output of the convert command so that the amount is printed
on the bucket account, not the guessed account.

2022/02/01* Blublub
    Expenses:Groceries                   100,00
    Accounts:Cache

becomes

2022/02/01* Blublub
    Expenses:Groceries
    Accounts:Cache                      -100,00

This makes subclassifying after conversion a bit easier

@tbm
Copy link
Contributor

tbm commented Feb 2, 2022

I'm not sure I see the advantage of this. Can you elaborate?

Why not put the amount on both postings; but maybe some people don't like that

@madroach
Copy link
Author

madroach commented Feb 2, 2022

When importing via csv you always get transactions with two postings. One is the posting on the real account the csv is about. The other one is guessed.
Now I often want something like this:

2022-02-02 Supermarket
    Assets:Credit Card    -50
    Expenses:Groceries     10
    Expenses:Household     20
    Expenses:Drinks        20
  • It is easier to get there if there is already an amount on the asset. Otherwise I need to move the amount from an arbitrary account to the asset and negate it. I want to avoid that.
  • It makes sense, because the csv was about the asset, not the guessed account. Indeed the other account has a calculated amount to balance the real asset account.

Putting the amount on both postings would be fine with me, but this would require a change to print.cc, which would affect more than just the output of convert.

@tbm
Copy link
Contributor

tbm commented Feb 3, 2022

Ok, thanks for the clarification. I agree with your points; otoh, people might not like the negative numbers that the change will cause (even if they accurately represent the transaction).

I think I'll leave this up for John to decide (but he has been busy).

You may want to email the ledger mailing list about this and gather feedback from other users.

@JesseFL
Copy link

JesseFL commented Apr 8, 2022

I agree with this change 100%. I think users will often want to split their income/expenses and moving the posted amount to the bank account would facilitate this. Further, the CSV is likely provided by a source for one account, the bank/credit account, and it would logically make sense for ledger convert to attach the amount in the CSV to the referenced account.

@tbm
Copy link
Contributor

tbm commented Apr 10, 2022

can someone email the ledger mailing list to discuss this change and see what the feedback is.

I think the arguments here are good, but since it's a change in behaviour I think it's better to discuss it more widely.

@JesseFL
Copy link

JesseFL commented Apr 15, 2022

@JesseFL
Copy link

JesseFL commented Apr 24, 2022

Only received one comment, but it was from @jwiegley, so it may be worth more than one vote ;)

Sounds like a great feature request to me!

John

@jwiegley
Copy link
Member

Could we add an option to do this flipping? It could change the workflow for many people.

@tbm
Copy link
Contributor

tbm commented May 5, 2022

@madroach what do you think of John's suggest to make it an option.

@madroach
Copy link
Author

sure, that would be fine with me.

@jwiegley
Copy link
Member

@madroach Great, just let me know when that change is in!

@afh
Copy link
Member

afh commented Feb 1, 2023

@madroach Mind merging the current state of master, which has seen some improvements regarding the checks run on PRs, to this PRs branch too?

@afh
Copy link
Member

afh commented Apr 12, 2023

Friendly ping for @madroach

mark the posting with the guessed account as calculated.
This will change the output so that the amount is printed
after the bucket account, not the guessed account.

2022/02/01* Blublub
    Expenses:Groceries                   100,00
    Accounts:Cache

becomes

2022/02/01* Blublub
    Expenses:Groceries
    Accounts:Cache                      -100,00

This makes subclassifying a bit easier
@madroach
Copy link
Author

madroach commented Jun 5, 2023

I rebased my change. Sorry for taking so long. @afh, @jwiegley

jwiegley
jwiegley previously approved these changes Jun 9, 2023
@jwiegley
Copy link
Member

jwiegley commented Jun 9, 2023

I like the change, but it seems to break a few tests? Can you confirm that these tests need to be changed now, and if so, include those changes here in the PR until it passes CI?

@afh afh self-assigned this Dec 9, 2023
@afh afh added the enhancement New feature or request label Dec 9, 2023
@afh afh mentioned this pull request Dec 9, 2023
@afh
Copy link
Member

afh commented Dec 9, 2023

I took the liberty to address the breaking tests. The changes can be found here. I'm ot sure how to best update this PR with these changes as this PR's branch is on a fork, hence I created a PR madroach/ledger#1.

@madroach would you be willing to accept that, so this PR's branch gets those changes and we can move forward with merging this PR?

@jwiegley Are you okay with the fixes (see Fix DocTests and tests: Fix Baseline and Regression tests) and are any changes to the documentation necessary?

merge updated test cases from afh/pr/2086
@madroach
Copy link
Author

Merged. Thanks for the work. I couldn't find the time for it...

@afh afh requested a review from jwiegley December 10, 2023 17:49
@afh
Copy link
Member

afh commented Dec 10, 2023

Thanks for merging, @madroach, very much appreciated! No worries regarding not finding the time, often enough other obligations stand in the way of the fun stuff 🙂

@jwiegley
Copy link
Member

I think this is set to go, it just still needs the option to flip the behavior, which should be relatively easy to add as a session_t option.

@afh
Copy link
Member

afh commented Dec 12, 2023

@madroach would you be okay with me taking over the work and adding the requested option as well as accompanying documentation and tests?

If yes, I'd close this PR in favor of one I'd create in order to more easily push changes to it. You'd be credited with Co-authored-by: @madroach on one of the commits unless you object.

@madroach
Copy link
Author

Yes, please do! I just won't find the time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants