redstreet / fava_investor

Comprehensive set of reports, analyses, and tools for investments, for Beancount and Fava (plain text, double entry accounting software). Includes Fava plugins, command line tools, and libraries for each module.
GNU General Public License v3.0
136 stars 20 forks source link

Fix for fava 1.2.5 #85

Closed tschicke closed 11 months ago

tschicke commented 1 year ago

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 commented 1 year ago

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

patbakdev commented 11 months ago

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

from fava.template_filters import cost_or_value
redstreet commented 11 months ago

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

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

redstreet commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

That's great, thank you for researching this!

tschicke commented 11 months ago

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.