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

Fix for fava 1.2.5 #85

Closed
wants to merge 4 commits into from
Closed

Fix for fava 1.2.5 #85

wants to merge 4 commits into from

Conversation

tschicke
Copy link
Contributor

This is initial work on #84

This probably shouldn't be merged as is, but this should be most of the changes required to get everything working with fava 1.2.5. The main changes are switching from convert.convert_position to fava's built-in convert_position, and some modifications to the charts api.
The one case I haven't fixed is in libassetalloc.py, under the if amount.currency == pos.units.currency and amount.currency != base_currency case. As far as I can tell, fava's convert_position doesn't support via, so I wasn't sure what to do with that block.

I also made it so that the generated currency regexes use ^ and $ so that substrings are not incorrectly matched.

Finally, I modified a few version checks to be able to detect development versions of fava. I have a situation where I am installing fava from a local repository, and the installed fava version ends up as 0.1.dev[commit hash], so the version checks are detecting an older version of fava. My thought was that if you are using a development version of fava, it's probably safe to assume that the fava version is up to date and the newest APIs should be used. Feel free to remove those sections of the change if you want, I can work around that on my end if you don't want to merge a sort of hacky check.

I also haven't used fava_investor before. I made these fixes as part of setting up fava_investor, so I don't have a reference for how everything looked/worked before fava 1.2.5 broke things, so apologies if some of the changes break some stuff that I didn't notice.

@redstreet
Copy link
Owner

Great - thanks a bunch for this PR! I'll take a look at it soon.

@patbakdev
Copy link

I think this import should also be removed from favainvestorapi.py

from fava.template_filters import cost_or_value

@redstreet
Copy link
Owner

redstreet commented Dec 12, 2023

Okay, I looked into this. To upgrade to Fava 1.25 and beyond:

  • Commit 86bcd2 in fava removes root_account. This breaks fava_investor, which needs an alternative that needs to be looked into.
  • Beancount Investor API also needs to be updated to account for upstream changes

@tschicke, thanks again for this PR. It's been very helpful in kickstarting the changes needed. I'll get back on this soon.

@redstreet redstreet force-pushed the main branch 2 times, most recently from d40fd56 to 79a10f4 Compare December 13, 2023 09:28
@redstreet
Copy link
Owner

With the latest fixes, everything works except for asset allocation by class, with which the display formatting is incorrect, and is missing the graph. I've opened this issue for it. Help appreciated.

@tschicke thanks again for helping jumpstart this.

I would still like to take your changes around dev versions and the currency. Would you mind please resolving the conflicts and resubmitting this PR, or opening a new PR? The 1.25 changes aren't needed any more.

Also, with the 'dev' versions, can we simply strip out the dev from the version string, and perhaps even replace it with a 0 if needed? This way, we aren't making the assumption that dev always is the most recent branch. Let me know if that makes sense, or if I'm mistaken in understanding the version string format.

@tschicke
Copy link
Contributor Author

Yeah I'll resolve the conflicts and remove the unneeded changes soon. I have to check the format for a dev version of fava. From what I remember it wasn't based on the non-dev version string, which is why I ended up just checking if it contains "dev" at all, but I'll confirm that. I'll also try to take a look at the asset allocation by class problem, but I'm not sure how much I'll be able to help with that.

@tschicke
Copy link
Contributor Author

Ok, fava's version string comes from attr: setuptools_scm.get_version, which I think determines the version based on the most recent tag in the git tree relative to the current commit. Since I was using my forked repository, but I hadn't synced any of the upstream tags, I was getting a version string of "0.1.dev2914+ge370230". After syncing the tags from upstream, I'm now getting a more proper version string of "1.24.5.dev48+ge370230a" (I need to update my fork to a more recent version). version.parse handles that version string properly, so I think the existing version logic actually works as is for dev versions, as long as you have the correct tags synced. So I'll remove that part of the change too, and just leave the currency change.

@redstreet
Copy link
Owner

That's great, thank you for researching this!

@tschicke
Copy link
Contributor Author

I opened a new PR for the currency changes since they have nothing to do with the original 'Fix for fava 1.2.5' purpose of this PR, so I'm closing this PR.

@tschicke tschicke closed this Dec 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants