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

Add real daily aggregation, fix small numbers #269

Merged
merged 11 commits into from
Jan 17, 2021

Conversation

AlexanderLill
Copy link
Contributor

Hi @ChrisRBe,

vielen Dank noch einmal für den tollen Parser. Ich habe eine neue Aggregation hinzugefügt. Damit heisst die alte "daily" aggregation jetzt "transaction", da jede Transaktion 1:1 übernommen wird, und die neue "daily" erzeugt pro Transaktionstyp und Tag einen Eintrag.

Weiterhin habe ich mit einem str.format versucht das richtige Format sehr kleiner Zahlen sicherzustellen, da manchmal das wissenschaftliche Format (z.B. "1,247e-06") in die CSV geschrieben wurde, und Portfolio Performance damit ein Problem hat.

Without formatting sometimes scientific formats (e.g. "1,247e-06") are used by default, which cause issues when imported into Portfolio Performance.
Copy link
Owner

@ChrisRBe ChrisRBe left a comment

Choose a reason for hiding this comment

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

Könntest du bitte noch mal schauen, ob die Readme Angebot angepasst werden muss?
Der neue Aggregationstyp ist noch nicht in den unit tests mit drin. Ich schaue später mal rein.

Werde mir auch noch mal die buildfehler anschauen.

src/portfolio_performance_writer.py Outdated Show resolved Hide resolved
@@ -53,6 +53,26 @@ def config_file(self, value):
"""config file property setter"""
self._config_file = value

def __aggregate_statements_daily(self, formatted_account_entry):
Copy link
Owner

Choose a reason for hiding this comment

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

Schaut gut aus.

Ich denke, dass der parser noch mal refaktoriert werden muss, um die gleichanteile loszuwerden. Eventuell was für später.

@AlexanderLill
Copy link
Contributor Author

AlexanderLill commented Jan 17, 2021

Könntest du bitte noch mal schauen, ob die Readme Angebot angepasst werden muss?

Man vergisst immer was... :) README ist jetzt angepasst.

Copy link
Owner

@ChrisRBe ChrisRBe left a comment

Choose a reason for hiding this comment

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

Looks good :)
Have to check the failing tests later.

@AlexanderLill
Copy link
Contributor Author

AlexanderLill commented Jan 17, 2021

Fixing the tests might be a bit more complicated, as they are trying to feed strings into the newly introduced integer formatting function... I "fixed" it, but I am not sure you will be happy about it...

@ChrisRBe
Copy link
Owner

Will have a look in a bit once I'm at my desk.

logging.debug("entry type is {}. new entry date is {}".format(entry_type, entry_date))
if entry_date not in self.aggregation_data:
self.aggregation_data[entry_date] = {}
if entry_type in self.aggregation_data[entry_date]:
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 2 locations. Consider refactoring.

@codeclimate
Copy link

codeclimate bot commented Jan 17, 2021

Code Climate has analyzed commit 1b1f0fc and detected 2 issues on this pull request.

Here's the issue category breakdown:

Category Count
Duplication 2

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 98.9% (0.1% change).

View more on Code Climate.

@ChrisRBe
Copy link
Owner

Agh. Ok... seems that we get a little screwed here by ScaCap/action-surefire-report#31 ... will update the workflow to remove the whole step for now. After PR is merged will re-enable it.

@ChrisRBe ChrisRBe merged commit f1f6d77 into ChrisRBe:master Jan 17, 2021
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.

2 participants