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

feat: add support for use in importer hooks #16

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

blaggacao
Copy link

@blaggacao blaggacao commented Apr 28, 2024

Context

CONFIG = [
  apply_hooks(bank_account, [
    ReconcileExpected("expected-incoming.beancount"),
    PredictPostings(),
  ])
]
script_utils.ingest(
  CONFIG,
  hooks=[
    ReconcileExpected.adapted_find_duplicate_entries,
  ],
)

@blaggacao blaggacao marked this pull request as ready for review April 28, 2024 11:58
@blaggacao
Copy link
Author

@maread99 The third commit is just necessary a necessary fix so that I can operate beanahead in my setting. Against all odds, I decided to use global state, because the call stack is deep and the main ledger is part of a global context during runtime.

@blaggacao blaggacao marked this pull request as draft April 28, 2024 13:04
@maread99
Copy link
Owner

maread99 commented Apr 30, 2024

Hi @blaggacao. Thank you for this PR. Much appreciated!

I don't think I'm not going to get a chance to have a good look before next week. I will do then if not before.

I ran the tests although it's being pulled up on the linting.

Cheers
Marcus

@maread99 maread99 added the enhancement New feature or request label Apr 30, 2024
@blaggacao blaggacao marked this pull request as ready for review April 30, 2024 16:03
@blaggacao blaggacao marked this pull request as draft May 1, 2024 06:04
@blaggacao blaggacao marked this pull request as ready for review May 1, 2024 06:07
@blaggacao
Copy link
Author

blaggacao commented May 1, 2024

@maread99 Ok, I ensured linting, cleaned up the history and use this -as is- in my own production, now. Thanks for considering this contribution!

You could also cherry-pick the first commit as an independent fix.

This solves the problem of incompatible account names errors as soon as
one of the secified options set a different base account name, which
currently renders beanahead incompatible with the use of those options
Copy link
Owner

@maread99 maread99 left a comment

Choose a reason for hiding this comment

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

Thanks again for this @blaggacao. I've had a look, am grateful for the PR and in principle more than happy to merge the new functionality.

I've made a few comments and requests for info / changes. A few of the tests are failing although at least some should come good if the points I've raised are addressed.

Cheers

src/beanahead/rx_txns.py Outdated Show resolved Hide resolved
src/beanahead/reconcile.py Show resolved Hide resolved
RootAccountsContext = {} # global context


def set_root_accounts_context(path_ledger: str) -> dict[str]:
Copy link
Owner

Choose a reason for hiding this comment

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

Would you mind adding a simple test to verify that all's working as expected when users are using non-default category names?

from .errors import BeanaheadWriteError


class ReconcileExpected:
Copy link
Owner

Choose a reason for hiding this comment

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

Could a simple test be added to verify the hook is working as intended?

src/beanahead/scripts/cli.py Show resolved Hide resolved
@blaggacao
Copy link
Author

For the tests:

  • How do I install the environment locally (project-level, not global)?
  • How do I run the tests? (just pytest, correct?)

@maread99
Copy link
Owner

maread99 commented May 8, 2024

To a fresh (or existing) python environment (>=3.9) you could install the current version with the optional 'dev' dependencies:

>>> pip install beanahead[dev]

Then uninstall beanahead:

>>> pip uninstall beanahead

And install your fork. If you have your fork installed locally then from the project directory (containing pyproject.toml):

>>> pip install -e .

Otherwise:

>>> pip install git+https://github.com/blaggacao/beanahead.git#egg=beanahead

Or, I'd have thought you might(?) be able to do the whole lot with either:

>>> pip install -e .[dev]

or

>>> pip install git+https://github.com/blaggacao/beanahead.git#egg=beanahead[dev]

As for the tests, yup, pytest.

(I use VSCode which nicely integrates both the creation of environments and testing with pytest.)

Moves all remaining output to stderr.

Also changes `RootAccountsContext` to only include names
that are non-default.
@maread99
Copy link
Owner

maread99 commented May 9, 2024

Hi @blaggacao, I've added a commit which gets all the existing tests passing.

If, when you can, you could add tests to cover the new functionality and then I think we'll be good to merge.

Cheers
Marcus

@blaggacao
Copy link
Author

Thank you for the additions!

I've high load, atm, and will ll try to revisit next week.

@maread99
Copy link
Owner

Hi @blaggacao

I've made some changes to make beanahead compatible with beancount v3. As it is this PR isn't compatible with v3 but I suspect it would just be a matter of changing the dependency on beancount.ingest to beangulp. Although, I'm not sure if smart-importer is v3 compatible(?).

In any event, I think I'm leaning towards not implementing the smart-importer hook directly in beanahead. (This is principally because I don't use smart-importer and would be unable to provide any ongoing maintenance to the hook.) What I have done is implemented the following aspects of this PR:

These changes are included in the new v0.3 release. The new Options section of the README provides the documentation together with the note at the end of the Making beanahead files section.

I was thinking it might be best if you publish the hook yourself and we include a reference and link on the README here? With the changes above now made I'd have thought this could be pretty simple?

Let me know what you think.

Cheers
Marcus

@blaggacao
Copy link
Author

Hey, this sounds great!

For expectation management, my setup is currently working and I won't touch it in a while (maybe a year or so).

But I've taken note and will seek an occasion to conform to your suggestion, which I agree with and find reasonable.

Let's maybe keep this open just for reference, in the meantime? (Just for the sake of not burying it inside the "closed" stack of PRs).

@maread99
Copy link
Owner

maread99 commented Jul 1, 2024

Great, let me know if/when you do set something up and we can add a reference and link to the README here.

I'll leave this PR open in the meantime.

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.

2 participants