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 InvestStatmentLine as possible return from parse_record to fix typing check errors. #263

Open
wants to merge 41 commits into
base: master
Choose a base branch
from

Conversation

jaik03
Copy link
Contributor

@jaik03 jaik03 commented Jan 14, 2024

A StatementParser parse_record call should return either a StatementLine or InvestStatementLine upon call. Added missing InvestStatementLine type to return to silence typing check errors.

@@ -53,7 +53,7 @@ def split_records(self) -> Iterable[LT]: # pragma: no cover
"""Return iterable object consisting of a line per transaction"""
raise NotImplementedError

def parse_record(self, line: LT) -> Optional[StatementLine]: # pragma: no cover
def parse_record(self, line: LT) -> Optional[StatementLine|InvestStatementLine]: # pragma: no cover
Copy link
Owner

Choose a reason for hiding this comment

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

Hm, I somehow missed the moment when this InvestStatementLine was introduced (found it now in #129), but I'm not a big fan of these ambiguous return type.

What typing check errors are you trying to address with this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

error: Return type "InvestStatementLine | None" of "parse_record" incompatible with return type "StatementLine | None" in supertype "StatementParser" [override]

I'm building a plugin to parse a JSON file filled with investment information, and mypy is throwing an override error due to the fact that my declaration says I'm returning an InvestStatementLine.

I neglected to notice that the | syntax was introduced in 3.10, and I think I used it incorrectly anyways. I can fix the PR, but do you want to approach this a different way? I can always have it ignore that specific type check in my parser code, but I thought it was better to address

kedder and others added 5 commits January 17, 2024 21:56
We can't get rid of them, because all the plugins rely on this namespace
style.
Return pkg_resources namespaces back
Bumps [pytest](https://github.com/pytest-dev/pytest) from 7.4.4 to 8.0.0.
- [Release notes](https://github.com/pytest-dev/pytest/releases)
- [Changelog](https://github.com/pytest-dev/pytest/blob/main/CHANGELOG.rst)
- [Commits](pytest-dev/pytest@7.4.4...8.0.0)

---
updated-dependencies:
- dependency-name: pytest
  dependency-type: direct:development
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>
@kedder
Copy link
Owner

kedder commented Feb 4, 2024

Oh, I forgot about this. Basically we need to find a better way here.

I think parse_record() should return StatementLine and nothing else. There should be a different method to return InvestStatementLine. I was planning to look closer at it, but forgot.

@coveralls
Copy link

coveralls commented Feb 4, 2024

Coverage Status

coverage: 96.05% (+0.3%) from 95.74%
when pulling 9688f8f on jaik03:AddInvestStatementLineType
into 0e20017 on kedder:master.

@jaik03
Copy link
Contributor Author

jaik03 commented Feb 4, 2024

Oh, I forgot about this. Basically we need to find a better way here.

I think parse_record() should return StatementLine and nothing else. There should be a different method to return InvestStatementLine. I was planning to look closer at it, but forgot.

Right, this was on the back burner for me and I was just fixing it to build correctly before I started digging into a better solution. I also set up a separate test environment to run through this properly so I'm not spamming you with Github workflow failures, but accidentally pushed to the wrong repo (Sorry!). Based on what you wrote I'll take a look and see if I can see any solutions that jump out at me.

kedder and others added 19 commits February 11, 2024 00:36
Bumps [pytest](https://github.com/pytest-dev/pytest) from 8.0.0 to 8.0.1.
- [Release notes](https://github.com/pytest-dev/pytest/releases)
- [Changelog](https://github.com/pytest-dev/pytest/blob/main/CHANGELOG.rst)
- [Commits](pytest-dev/pytest@8.0.0...8.0.1)

---
updated-dependencies:
- dependency-name: pytest
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
For some investment transactions (e.g. INTEREST) there is no security,
so don't add a blank <STOCKINFO> that will get rejected by Gnucash.
Total doesn't need more than 2 decimal places.
Bumps [zipp](https://github.com/jaraco/zipp) from 3.8 to 3.17.0.
- [Release notes](https://github.com/jaraco/zipp/releases)
- [Changelog](https://github.com/jaraco/zipp/blob/main/NEWS.rst)
- [Commits](jaraco/zipp@v3.8.0...v3.17.0)

---
updated-dependencies:
- dependency-name: zipp
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [pytest](https://github.com/pytest-dev/pytest) from 8.0.1 to 8.0.2.
- [Release notes](https://github.com/pytest-dev/pytest/releases)
- [Changelog](https://github.com/pytest-dev/pytest/blob/main/CHANGELOG.rst)
- [Commits](pytest-dev/pytest@8.0.1...8.0.2)

---
updated-dependencies:
- dependency-name: pytest
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
For INTEREST, XFER, and other cash transactions that are not
associated with a security, INVBANKTRAN is the appropriate
transaction type.

The body is like a bank transaction (STMTTRN), not an INVTRAN.
For share transfers, TRANSFER is the appropriate type with no
trntype_detailed, no amount and an optional unit_price.
…ents

Handle additional types of investment transactions
After the switch to importlib, plugins are retrieved by name,
not integer index.
For details see importlib.metadata.EntryPoints.__getitem__()

Also update the mock.patch to match: entry_points() -> EntryPoints
dependabot bot and others added 15 commits March 16, 2024 20:36
Bumps [mypy](https://github.com/python/mypy) from 1.8.0 to 1.9.0.
- [Changelog](https://github.com/python/mypy/blob/master/CHANGELOG.md)
- [Commits](python/mypy@v1.8.0...1.9.0)

---
updated-dependencies:
- dependency-name: mypy
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [black](https://github.com/psf/black) from 24.2.0 to 24.3.0.
- [Release notes](https://github.com/psf/black/releases)
- [Changelog](https://github.com/psf/black/blob/main/CHANGES.md)
- [Commits](psf/black@24.2.0...24.3.0)

---
updated-dependencies:
- dependency-name: black
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [zipp](https://github.com/jaraco/zipp) from 3.17.0 to 3.18.1.
- [Release notes](https://github.com/jaraco/zipp/releases)
- [Changelog](https://github.com/jaraco/zipp/blob/main/NEWS.rst)
- [Commits](jaraco/zipp@v3.17.0...v3.18.1)

---
updated-dependencies:
- dependency-name: zipp
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [pytest](https://github.com/pytest-dev/pytest) from 8.0.2 to 8.1.1.
- [Release notes](https://github.com/pytest-dev/pytest/releases)
- [Changelog](https://github.com/pytest-dev/pytest/blob/main/CHANGELOG.rst)
- [Commits](pytest-dev/pytest@8.0.2...8.1.1)

---
updated-dependencies:
- dependency-name: pytest
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [types-mock](https://github.com/python/typeshed) from 5.1.0.20240106 to 5.1.0.20240311.
- [Commits](https://github.com/python/typeshed/commits)

---
updated-dependencies:
- dependency-name: types-mock
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
…srv/git/python-ofxstatement into AddInvestStatementLineType
@jaik03
Copy link
Contributor Author

jaik03 commented Mar 31, 2024

So I finally got around to looking at this again. I duplicated out parse_record into a parse_invest_record, and it
seems to work just fine with the plugin I'm developing.

The only part I'm unsure on is having the 'NotImplementedError' included. Since it's up to the plugin author to declare parse(), it'll be incumbent on them to declare either parse_record or parse_invest_record as appropriate, but it won't throw a typing error.

@@ -0,0 +1,39 @@
[build-system]
requires = ["setuptools>=61.0"]
build-backend = "setuptools.build_meta"
Copy link
Owner

Choose a reason for hiding this comment

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

Hm, something is messed up. This shouldn't be part of the diff. Can you rebase on master or merge it into your branch?

@@ -46,7 +46,7 @@ def parse(self) -> Statement:
stmt_line = self.parse_record(line)
if stmt_line:
stmt_line.assert_valid()
self.statement.lines.append(stmt_line)
self.statement.lines.append(stmt_line) # type: ignore
Copy link
Owner

Choose a reason for hiding this comment

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

This # type: ignore is not needed now?

FWICT, Statement class has two lists:

class Statement(Printable):
    lines: List["StatementLine"]
    invest_lines: List["InvestStatementLine"]

Plugins that parse investment statement are supposed to add lines to invest_lines. Those that parse regular bank statemrents add to lines.

Someone who added InvestStatementLine feature probably just overrode parse() and didn't use this implementation at all. And we didn't bother to add the same level of support for the investment statements that regular statements have.

That said, I think parse_invest_record is the move in right direction, we just need to use it here somehow, because right now it looks like a lonely unused method. What I suggest is to return None by default from both parse_record() and parse_invest_record(), indicating they wasn't able to parse the record.

Then, in parse itself add another couple of lines:

if stmt_line:
    ...
else:
    invest_stmt_line = self.parse_invest_record(line)
    if invest_stmt_line:
        self.statement.invest_lines.append(invest_stmt_line)

This way users can override one of the parse_*_record depending on what statements their plugins are parsing and still make use of the generic parse() method.

And we won't need these shameful # type: ignore comments as well :)

What do you think?

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.

4 participants