-
Notifications
You must be signed in to change notification settings - Fork 39
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
pdfreader and bamboohr paycheck importer #94
pdfreader and bamboohr paycheck importer #94
Conversation
Looks great, and I'd be happy to take this in. Thanks for the helpful comments and the debug option too. I completely understand the challenge in adding any kind of examples at all to importers since by definition they're all personal data. But I'm wondering if perhaps adding an example pdf that you even made up in any word processing program that is importable by this? I just want to ensure we have some way down the line to test code to ensure it's still alive. It doesn't have to use the bamboohr importer (though that'd be great too). Perhaps a simple single table made in MS-Word or the likes, and a simple dummy importer for it? |
For dependencies, the following should work:
|
|
||
self.alltables = {} | ||
for table in tables: | ||
self.alltables[table['section']] = etl.fromdataframe(pd.DataFrame(table['table'][1:], columns=table['table'][0])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I gave this a try and the table extraction worked for a PDF with 10 separate tables. If you want to avoid the dependency on pandas
this could be changed to something like this:
for table in tables:
t = table["table"]
# transpose table to use fromcolumns
tbl_t = [[t[j][i] for j in range(len(t))] for i in range(len(t[0]))]
self.alltables[table["section"]] = etl.fromcolumns(tbl_t)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we can have it work without pandas
that would be great. I'll implement you suggestion and try to get a test case up before the end of the week.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out we can just run etl.wrap(table['table'])
rather than preprocessing it.
I think the dependencies would just need added here |
Apologies for not getting back to this. I'm hoping to make the changes and add a test before the end of the week. |
requirements.txt
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why pigar changed the file so much. It appears to still work, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's moved all versions forward based on what is installed on your system. That's fine, I wouldn't worry about it much. requirements.txt
is used only to setup development environments, including by the github action to build the beancount_reds_importers package.
The package's dependencies for users who install it (i.e., for regular users) are in setup.py
which you will have to update manually. See install_requires
under it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the build is failing because pigar removed some dependencies. I'll look into it tomorrow and see what I can do.
I added a generic pdf paycheck importer that really isn't meant to be used but to show how to make a paycheck importer using the pdfreader. Tests are included and passing. |
This looks great thank you, and thanks for the test! Looks like the package build fails. If you could please take a look and fix that, we can get this merged in. |
I forgot to do this. I'll update it tomorrow! |
I wasn't able to get to this before I went off grid for a week. I'll try and update in the next few days. |
I'm not sure what happened, but pigar would not detect imports from other tests. So I manually updated the requirements.txt to include all needed files.
@redstreet Not sure why, but I could not get pigar to find all the dependencies necessary, so I just manually updated the |
Thanks, no idea why pigar fails, but updating requirements.txt is fine. The formatting is still failing, see above. If you could fix this, we can get this in. https://github.com/redstreet/beancount_reds_importers/blob/main/CONTRIBUTING.md shows what to run to fix formatting. |
Not sure why, but when I ran the formatting commands locally I got 64 changes instead of 3 as observed in the github action output. I'm working on a mac and for some reason the default ruff settings are different. I ended up running a docker image that copied the github action environment and ran the format commands inside docker and that got the correct formatting changes. Thanks for your patience, by the way. |
@a0js, curious, is there a button to run the formatting workflow that appears for you in this PR? I made a commit yesterday to enable this and am wondering if it works. |
Thanks for reporting. Hmm, not sure why you're seeing this. Ruff uses settings frm
Of course, no worries at all, thank you for sticking with this PR and getting it in, much appreciated! |
I don't see anything on the PR page, but I could be looking in the wrong place. Where should it be? |
I'd expect it to be on the bottom of the PR page. I'm sure it'd be an obvious green button, so if you haven't seen it, it must've not worked. Anyway, it doesn't matter much for this PR, thanks for checking! Do let me know if you need help with any of the outstanding things. |
I can't merge this myself as I don't have write access. If you think this is good to go, can you merge it in? |
I think it was failing checks, but the checks are not running now for some reason. Let me take a look. |
Checks are passing for me locally. I don't know why github wouldn't run them. Either way, merged! Thank you again for the contribution, and for working to get this PR through! IMO, table extraction from pdfs is a solid contribution for |
You're most welcome! I'm glad I could add something to this awesome project. Let me know if there are some other features I can help with. |
Sorry to comment on the old PR, but I was just curious how often you cut releases and when this one might be published? |
Np at all. I usually put it through personal use of at least a few weeks before I publish, so bugs have a chance to surface. Let me take a look this evening to see if I can cut a release. |
Released 0.9.0, featuring this PR :-) |
Purpose
Many paycheck downloads only come in the form of pdf. To support this use case, and potentially other use cases, we need to add support for pdf parsing. As a proof of concept, I also added the bambooHR paystub importer.
Approach
Outstanding Issues
Resolves #93