Skip to content

refactor: split enums into Action and StockMarketOperation (#53)#59

Open
przemyslawbialon wants to merge 2 commits intomainfrom
refactor/53-unify-action-operationtype
Open

refactor: split enums into Action and StockMarketOperation (#53)#59
przemyslawbialon wants to merge 2 commits intomainfrom
refactor/53-unify-action-operationtype

Conversation

@przemyslawbialon
Copy link
Copy Markdown
Collaborator

@przemyslawbialon przemyslawbialon commented Apr 21, 2026

Closes #53. Unblocks #9 sub-issues (#56, #57, #58).

What changed from the first version of this PR

The first approach merged OperationType and Action into a single 5-value Action enum. Review feedback rightly pointed out that this loses a domain boundary: Transaction(action=Action.STOCK_SPLIT) would be syntactically valid, and CsvValidator (once wired in #9) would let a crypto CSV carry STOCK_SPLIT rows — neither makes sense.

This revised version keeps a clean separation:

  • Action(BUY, SELL) — the transactional action, attribute of Transaction. Common to stocks and crypto.
  • StockMarketOperation(BUY, SELL, DIVIDEND, SERVICE_FEE, STOCK_SPLIT) — classifier for rows in a stock-market CSV. Stock-only; crypto CSVs use Action directly because their row type coincides with transaction action.

The silent .value-based coercion flagged in TAX_LAW_AUDIT.md Issue #1 is replaced by an explicit StockMarketOperation.to_action() method that raises for non-transactional values.

Files touched (14)

  • New: pit38/domain/stock/operations/stock_market_operation.py — enum with is_transaction() and to_action() helpers
  • pit38/domain/transactions/action.pyBUY/SELL only; add __hash__ (custom __eq__ requires it for set membership, e.g. TransactionRowParser.OPERATIONS_HANDLED)
  • pit38/domain/stock/operations/operation.py — drop obsolete OperationType class; keep thin Operation base
  • pit38/domain/stock/operations/{dividend,service_fee,stock_split}.pytype = StockMarketOperation.X
  • pit38/data_sources/stock_loader/factory.py — key by StockMarketOperation; call .to_action() explicitly when building Transaction
  • pit38/data_sources/stock_loader/csv_loader.py — parse column into StockMarketOperation
  • pit38/plugins/stock/revolut/{row,transaction_row,operation_row}_parser.py — migrated
  • pit38/stock.pyfilter_transactions uses isinstance(Transaction); other filter_* methods compare against StockMarketOperation.X
  • tests/test_csv_validator.py — asserts original "BUY, SELL" error message (validator wired for crypto stays 2-value)
  • TAX_LAW_AUDIT.md — Issue Create a service for business logic of stock transactions  #1 marked resolved, with explanation
  • .gitignore — add .coverage, .coverage.*, htmlcov/

Why __hash__?

Action.__eq__ is custom (compares .value). Python requires __hash__ when __eq__ is overridden, or the enum becomes unhashable and can't be used in sets. This matters for e.g. TransactionRowParser.OPERATIONS_HANDLED = {StockMarketOperation.BUY, StockMarketOperation.SELL}. Added on both enums to be safe.

Verification

  • .venv/bin/pytest tests/ → 92 passed
  • pit38 stock -f example_format.csv -y 2025 → same output as main (profit +2 261.13 PLN, tax 429.61 PLN)
  • pit38 crypto -f example_format.csv -y 2025 → same output as main (loss 29 271.00 PLN, tax 0)
  • grep -r OperationType pit38/ tests/ returns only local BinanceOperationType (unrelated)
  • grep -rE "Action\.(DIVIDEND|SERVICE_FEE|STOCK_SPLIT)" pit38/ tests/ returns nothing — the semantic boundary holds
  • Sanity: Action.STOCK_SPLIT raises AttributeErrorTransaction.action cannot be assigned a non-transactional value

Related

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 21, 2026

Codecov Report

❌ Patch coverage is 77.58621% with 13 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
.../domain/stock/operations/stock_market_operation.py 77.27% 4 Missing and 1 partial ⚠️
pit38/domain/transactions/action.py 25.00% 2 Missing and 1 partial ⚠️
...it38/plugins/stock/revolut/operation_row_parser.py 50.00% 3 Missing ⚠️
pit38/data_sources/stock_loader/factory.py 66.66% 1 Missing ⚠️
pit38/stock.py 83.33% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

OperationType and Action previously had overlapping BUY/SELL values,
relying on `.value` equality to coerce between them in the stock loader
factory — flagged in TAX_LAW_AUDIT.md as fragile.

Rather than merging them into a single 5-value enum (which would let
Transaction.action be set to DIVIDEND/SERVICE_FEE/STOCK_SPLIT — nonsense
semantically, and would weaken per-asset-class validation), this splits
the responsibility by domain:

- `Action(BUY, SELL)` — the transaction action. Common to stocks and
  crypto. Attribute of Transaction.
- `StockMarketOperation(BUY, SELL, DIVIDEND, SERVICE_FEE, STOCK_SPLIT)` —
  classifier for rows in a stock-market CSV. Stock-specific.

Crypto CSVs do not use StockMarketOperation — their row type and the
transaction action coincide, so they use Action directly.

Silent coercion in the stock factory is replaced by an explicit
`StockMarketOperation.to_action()` method that raises for
non-transactional values.

Changes:
- pit38/domain/transactions/action.py: keep BUY/SELL only; add __hash__
  (needed because of custom __eq__, e.g. for set membership in
  TransactionRowParser.OPERATIONS_HANDLED)
- pit38/domain/stock/operations/stock_market_operation.py: new enum
  with is_transaction() and to_action() helpers
- pit38/domain/stock/operations/{dividend,service_fee,stock_split}.py:
  type = StockMarketOperation.X
- pit38/data_sources/stock_loader/factory.py: use StockMarketOperation
  as dict key; call .to_action() explicitly when building a Transaction
- pit38/data_sources/stock_loader/csv_loader.py: parse column into
  StockMarketOperation
- pit38/plugins/stock/revolut/{row,transaction_row,operation_row}_parser.py:
  migrated to StockMarketOperation
- pit38/stock.py: filter_* classmethods use StockMarketOperation for
  stock-specific filters; filter_transactions uses isinstance(Transaction)
- TAX_LAW_AUDIT.md: Issue #1 marked resolved with explanation of the
  two-enum approach

Closes #53. Unblocks #9 sub-issues (#56, #57, #58) — the BaseCsvLoader
ABC can be generic over either enum, and validators naturally receive
the right subset per asset class.
Adds .coverage, .coverage.*, and htmlcov/ to .gitignore so pytest-cov
output doesn't show as untracked on every dev's machine.
@przemyslawbialon przemyslawbialon changed the title refactor: unify OperationType into Action enum (#53) refactor: split enums into Action and StockMarketOperation (#53) Apr 21, 2026
@przemyslawbialon przemyslawbialon force-pushed the refactor/53-unify-action-operationtype branch from 6f5d60c to 9df479b Compare April 21, 2026 11:32
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.

refactor: Unify OperationType and Action enums

1 participant