Add regexes to ignore entries and evaluate special cases #280

AlexanderLill · 2021-02-15T07:49:23Z

No description provided.

codeclimate · 2021-02-15T07:50:02Z

Code Climate has analyzed commit 3f1dca4 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 99.3% (0.3% change).

View more on Code Climate.

ChrisRBe

Let me know your thoughts on my comments :)

ChrisRBe · 2021-02-16T20:13:34Z

src/p2p_account_statement_parser.py

@@ -108,6 +108,9 @@ def __format_statement(self, statement):
        if not category:
            return

+        if category == "Ignored":


I think you can move this up to the statement in line 108:

if not category or category == "Ignored"

done, thanks :)

ChrisRBe · 2021-02-16T20:16:08Z

src/Statement.py

@@ -46,8 +56,11 @@ def is_fee(self, booking_type):

    def get_date(self):
        """ get the date of the statement """
+        date_to_parse = self._statement[self._config.get_booking_date()]
+        if date_to_parse == "":


Suggested change

if date_to_parse == "":

if not date_to_parse:

empty strings are usually evaluated as None in python. I don't know if you knew that already :)

Refactored so that does not show up anymore. I knew, but sometimes I tend to be overly explicit with these if statements ^^

ChrisRBe · 2021-02-16T21:53:28Z

src/Statement.py

@@ -27,6 +27,7 @@ def get_category(self):
        :return: category of the statement; if unknown return the empty string
        """
        booking_type = self._statement[self._config.get_booking_type()]
+        value = self.get_value()


Hm. this variable is only read twice. could be discussed if necessary.

Refactored so that does not show up anymore, but I agree, it was probably not necessary

ChrisRBe · 2021-02-16T22:04:00Z

src/Statement.py

+        elif self._config.get_special_entry_regex() and self._config.get_special_entry_regex().match(booking_type):
+            if value > 0:
+                category = "Zinsen"
+            elif value < 0:
+                category = "Gebühren"
+            else:
+                logging.debug(self._statement)
+        elif self._config.get_ignorable_entry_regex() and self._config.get_ignorable_entry_regex().match(booking_type):
+            category = "Ignored"


Maybe move the if statements to its own methods (see is_fee). This is mostly to reduce the complexity of the overall function.
This could look something like:

Suggested change

elif self._config.get_special_entry_regex() and self._config.get_special_entry_regex().match(booking_type):

if value > 0:

category = "Zinsen"

elif value < 0:

category = "Gebühren"

else:

logging.debug(self._statement)

elif self._config.get_ignorable_entry_regex() and self._config.get_ignorable_entry_regex().match(booking_type):

category = "Ignored"

elif self.is_special_entry_interest(booking_type):

category = "Zinsen"

elif self.is_special_entry_fee(booking_type):

category = "Gebühren"

elif self.is_ignorable(booking_type):

category = "Ignored"

def is_ignorable(self, booking_type):

return self._config.get_ignorable_entry_regex() and self._config.get_ignorable_entry_regex().match(booking_type)

def is_special_entry_interest(self, booking_type):

return self._config.get_special_entry_regex() and self._config.get_special_entry_regex().match(booking_type) and self.value() >= 0

def is_special_entry_fee(self, booking_type):

return self._config.get_special_entry_regex() and self._config.get_special_entry_regex().match(booking_type) and self.value() < 0

Overall (in retrospect) not too happy with how much this if branching explodes :) I should have come up with a better way to handle these different categories.

Thanks, I tried to make it a little more clear in the refactored version, let me know what you think :)

I also put the code for handling the special mintos case in a separate function, I am not too happy about the whole part, but the format of the csv does not make it easy to parse the category :|

ChrisRBe · 2021-02-16T22:05:10Z

src/Statement.py

@@ -46,8 +56,11 @@ def is_fee(self, booking_type):

    def get_date(self):
        """ get the date of the statement """
+        date_to_parse = self._statement[self._config.get_booking_date()]


Why is this required? Are there statements without date? not so nice. hmm.

I had a downloaded csv with a line without date, but in the file I downloaded today all entries have a date.

So we could keep it to be more resilient for missing dates, as otherwise the parser just throws an error and stops.

ChrisRBe · 2021-02-16T22:19:32Z

src/Statement.py

+        date_to_parse = self._statement[self._config.get_booking_date()]
+        if date_to_parse == "":
+            return datetime(1970, 1, 1).date()
        return datetime.strptime(
-            self._statement[self._config.get_booking_date()],
+            date_to_parse,
            self._config.get_booking_date_format(),
        ).date()


Hm. would this be cleaner?

if self._statement[self._config.get_booking_date()]: statement_date = datetime.strptime( self._statement[self._config.get_booking_date()], self._config.get_booking_date_format() ).date() else: statement_date = datetime(1970, 1, 1).date() return statement_date

Could you also add a sample entry to the mintos test data to verify that an empty entry is correctly processed? :)

Yes that looks a lot better, thanks!

AlexanderLill · 2021-02-17T08:54:30Z

@ChrisRBe thanks for the very useful feedback :) I might be able to work on it on the weekend and also add some tests. I saw the no-date entry the first time, but maybe it was some kind of incomplete transaction on mintos' side, it was the last (and most recent) row in my csv export.

…ntos_extended_parsing

AlexanderLill · 2021-02-20T15:15:23Z

From my side I would now wait for further feedback. Unfortunately the current version does not pass the codeclimate requirement for complexity 5 (it has complexity 6).

src/Statement.py

ChrisRBe · 2021-02-20T15:52:42Z

src/Statement.py

+        # This is currently a special case for the Mintos "discount/premium" secondary market transactions parsing,
+        # where an entry might be a fee or an income depending on its sign.


Suggested change

# This is currently a special case for the Mintos "discount/premium" secondary market transactions parsing,

# where an entry might be a fee or an income depending on its sign.

"""

This is currently a special case for the Mintos "discount/premium" secondary market transactions parsing,

where an entry might be a fee or an income depending on its sign.

:param value: how much money was returned/paid

:return: Zinsen if value >= 0; Gebühren if < 0; Ignored in any other case

"""

That looks better, thanks :)

ChrisRBe · 2021-02-20T15:53:17Z

src/Statement.py

+        if value >= 0:
+            return "Zinsen"
+        elif value < 0:
+            return "Gebühren"
+        else:
+            return "Ignored"


Please correct me if I'm wrong, the else statement is only for the case where value is not a number, right? hm. Should work I guess.

Yes exactly, it might be unexpected, but at least that way the "Undecided" will not make it into the next higher level where it is not handled.

ChrisRBe · 2021-02-20T15:54:55Z

Hey @AlexanderLill, looks pretty good already. only one or two minor comments. Let me have a look at codeclimate, what he is bitching about.

ChrisRBe · 2021-02-20T16:22:47Z

Hey @AlexanderLill, looks pretty good already. only one or two minor comments. Let me have a look at codeclimate, what he is bitching about.

I had another look and added a PR on your fork repository. Let me know what you think.

…sing

AlexanderLill · 2021-02-20T17:31:59Z

@ChrisRBe thanks for the feedback, I merged your branch into mine and implemented your feedback. For me this PR on github is getting a bit confusing (especially with the comments on code parts that have changed in the meantime) but I hope I did not forget anything :)

ChrisRBe · 2021-02-20T17:33:08Z

Okay. There's only 2 issues remaining on coding content regarding duplicate code in the writer. I accepted those already and really should have a look at later.

ChrisRBe · 2021-02-20T17:33:46Z

@ChrisRBe thanks for the feedback, I merged your branch into mine and implemented your feedback. For me this PR on github is getting a bit confusing (especially with the comments on code parts that have changed in the meantime) but I hope I did not forget anything :)

I will have another look, but it looks really good already :)

src/Statement.py

ChrisRBe · 2021-02-22T23:36:32Z

@AlexanderLill looks good overall now :) Let me know when you want to merge the PR. 👍

AlexanderLill · 2021-02-23T07:12:28Z

@ChrisRBe great, from my side it is ready to merge, feel free to do it whenever you can :)

Add regexes to ignore entries and evaluate special cases

3fec4fa

AlexanderLill mentioned this pull request Feb 15, 2021

Mintos Import: Summe Website & PP unterschiedlich #268

Closed

Merge branch 'master' into mintos_extended_parsing

5861296

ChrisRBe requested changes Feb 16, 2021

View reviewed changes

AlexanderLill and others added 6 commits February 17, 2021 09:56

Merge branch 'master' into mintos_extended_parsing

e22b8d2

Implement review feedback; create regex-category-mapping

861e74e

Merge remote-tracking branch 'origin/mintos_extended_parsing' into mi…

5684a04

…ntos_extended_parsing

Formatting regex-category-map: Make the linter happy

0d67ee6

Remove unused is_fee, refactor mintos special case handling

394d2b5

Fix return value documentation for get_category

8314ee1

ChrisRBe requested changes Feb 20, 2021

View reviewed changes

feat: further refactoring of category matching

706c04f

AlexanderLill added 5 commits February 20, 2021 18:16

Minor fixes to new __match_category function and mintos special case

207642f

Fix wrong 'Unexpected statement' messages

da1eb47

Merge branch 'mintos_extended_parsing_local' into mintos_extended_par…

e0a4295

…sing

Remove duplicate code for daily/monthly aggregation

d262e19

Change comment to correct doc syntax

9d29ffb

ChrisRBe requested changes Feb 20, 2021

View reviewed changes

src/Statement.py Outdated Show resolved Hide resolved

AlexanderLill and others added 3 commits February 20, 2021 20:51

Add test data for mintos discount/premium data (secondary market)

de693b8

fix: format code in test code

516a21f

Add more test data and removed unreached if branch

887ab50

ChrisRBe approved these changes Feb 22, 2021

View reviewed changes

Merge branch 'master' into mintos_extended_parsing

3f1dca4

AlexanderLill changed the title ~~WIP: Add regexes to ignore entries and evaluate special cases~~ Add regexes to ignore entries and evaluate special cases Feb 23, 2021

ChrisRBe merged commit 493fd68 into ChrisRBe:master Feb 23, 2021

AlexanderLill deleted the mintos_extended_parsing branch December 11, 2021 11:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add regexes to ignore entries and evaluate special cases #280

Add regexes to ignore entries and evaluate special cases #280

AlexanderLill commented Feb 15, 2021

codeclimate bot commented Feb 15, 2021 •

edited

Loading

ChrisRBe left a comment

ChrisRBe Feb 16, 2021

AlexanderLill Feb 20, 2021

ChrisRBe Feb 16, 2021

AlexanderLill Feb 20, 2021

ChrisRBe Feb 16, 2021

AlexanderLill Feb 20, 2021

ChrisRBe Feb 16, 2021

AlexanderLill Feb 20, 2021

AlexanderLill Feb 20, 2021

ChrisRBe Feb 16, 2021

AlexanderLill Feb 20, 2021

ChrisRBe Feb 16, 2021

AlexanderLill Feb 20, 2021

AlexanderLill commented Feb 17, 2021 •

edited

Loading

AlexanderLill commented Feb 20, 2021

ChrisRBe Feb 20, 2021

AlexanderLill Feb 20, 2021

ChrisRBe Feb 20, 2021

AlexanderLill Feb 20, 2021

ChrisRBe commented Feb 20, 2021

ChrisRBe commented Feb 20, 2021

AlexanderLill commented Feb 20, 2021

ChrisRBe commented Feb 20, 2021

ChrisRBe commented Feb 20, 2021

ChrisRBe commented Feb 22, 2021

AlexanderLill commented Feb 23, 2021

		# This is currently a special case for the Mintos "discount/premium" secondary market transactions parsing,
		# where an entry might be a fee or an income depending on its sign.

Add regexes to ignore entries and evaluate special cases #280

Add regexes to ignore entries and evaluate special cases #280

Conversation

AlexanderLill commented Feb 15, 2021

codeclimate bot commented Feb 15, 2021 • edited Loading

ChrisRBe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexanderLill commented Feb 17, 2021 • edited Loading

AlexanderLill commented Feb 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisRBe commented Feb 20, 2021

ChrisRBe commented Feb 20, 2021

AlexanderLill commented Feb 20, 2021

ChrisRBe commented Feb 20, 2021

ChrisRBe commented Feb 20, 2021

ChrisRBe commented Feb 22, 2021

AlexanderLill commented Feb 23, 2021

codeclimate bot commented Feb 15, 2021 •

edited

Loading

AlexanderLill commented Feb 17, 2021 •

edited

Loading