-
Notifications
You must be signed in to change notification settings - Fork 14
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
Add support for viainvest #457
Conversation
This adds support for parsing viainvest account statements. Withdrawals are currently not supported, only deposits and interest payments.
Any feedback on how to overcome the pipeline checks is very welcome :) |
Hi @AlexanderLill, Thanks for the PR first :) Depends on how you set up your local environment. I wanted to update the development section for some time already, but always find excuses not to do it. Here goes (this works for me under Windows, should also work in other environments): Some time ago I migrated most of the dev env from the requirements.txt file to pipenv. See also documentation here: https://pypi.org/project/pipenv/ pipenv basically sets up a virtualenv for you including black, pre-commit, flake, pylint, etc. If you don't want to do that (which is fine :)), the (virtual) env that is installed by both requirements.txt and Pipfile should deploy a local version of black. See https://github.com/psf/black. Inside your python environment you should be able to run black simply by invoking it via Running unit tests locally should also be as simple as running it via |
@@ -92,6 +93,23 @@ def get_currency(self): | |||
return self._statement[self._config.get_booking_currency()] | |||
else: | |||
return "EUR" | |||
|
|||
@staticmethod | |||
def _parse_value(value): |
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.
doc string missing :)
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.
Thanks, fixed :)
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.
Nice. Thank you. Almost perfect :). The value param description is missing
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.
True! Too bad that black does not check for this. I also found nothing in the documentation about adding this check :(
Code Climate has analyzed commit 3702e69 and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 100.0% (50% is the threshold). This pull request will bring the total coverage in the repository to 100.0% (0.7% change). View more on Code Climate. |
I'm also looking into the codeclimate failure. Looks like codeclimate thinks that it should also consider the test code for overall coverage. Which is eh... not what we want. I probably have to update one additional config file to fix this properly. |
@ChrisRBe from my side I don't see anything left to fix, even though the codeclimate check is failing. When I click on details it shows no issues, very strange :( |
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.
Looks good. one more change :)
Yea, I see the same .. I believe it's caused by coverage also reporting on the unit test files.. I have an open PR #460 to address this hopefully. We might have to update this PR via merge and run another check. |
bors merge |
457: Add support for viainvest r=ChrisRBe a=AlexanderLill This adds support for parsing viainvest account statements. Withdrawals are currently not supported, only deposits and interest payments. Currently I can't get the tests to run on my environment. `@ChrisRBe` is there a commandline command to run all tests? Edit: I just saw that also there are formatting issues, is there a command to run this locally or a nice way to show how the formatting should look like? :) Co-authored-by: Alexander Lill <[email protected]>
Build failed: |
bors merge |
Build succeeded: |
This adds support for parsing viainvest account statements. Withdrawals are currently not supported, only deposits and interest payments.
Currently I can't get the tests to run on my environment. @ChrisRBe is there a commandline command to run all tests?
Edit: I just saw that also there are formatting issues, is there a command to run this locally or a nice way to show how the formatting should look like? :)