fix(ownership): shares_traded counts zero market trades instead of raising (#1359) - #1367
Merged
dgunning merged 1 commit intoSep 28, 2026
Conversation
…ising (dgunning#1359) `NonDerivativeTable.market_trades` returns None when the non-derivative table has no transactions, and an empty frame when it has transactions but none of them is coded P or S. Every caller of it guards both states - `market_trades` itself in the summary path, `common_stock_purchases`, `common_stock_sales`, `get_ownership_summary` - except `Ownership.shares_traded`, which read `.Shares` straight off it. So a derivative-only Form 4 parsed fine, returned its derivative transaction through `to_dataframe()`, and then raised `AttributeError: 'NoneType' object has no attribute 'Shares'` on the accessor. Vertex Pharmaceuticals' Form 4 filed 2020-10-19 (accession 0001209191-20-055264) is one, and it is already the fixture behind `tests/ownership/test_form4.py::test_form4_with_derivatives`. `shares_traded` now returns 0 for both states. 0 is the count of non-derivative open-market trades and says nothing about derivative activity, which this accessor has never counted - the docstring says so, because the number is easy to misread as "nothing happened". The award-only case already returned 0 by summing an empty numeric column, so that answer is unchanged; the empty branch now also covers an empty frame whose Shares column is not numeric, which fell through to None before. The nonempty non-numeric fall-through to None is deliberately untouched: a filing that traded an amount we cannot parse must not be reported as having traded nothing. A regression test pins that alongside the two controls. The test reads a checked-in copy of the Vertex XML and stubs the reporting-owner CIK lookup, the only fetch in `Ownership.parse_xml`, using the same stub as `tests/ownership/test_insider_parse_contract.py`. Verified offline under `pytest -p tests._offline_harness`, so it gates pull requests rather than landing in the post-merge network lane. Fixes dgunning#1359 Co-Authored-By: Claude <noreply@anthropic.com>
manantlerio
force-pushed
the
fix/1359-shares-traded-derivative-only
branch
from
September 26, 2026 16:43
911696c to
fb882e0
Compare
monody0007
added a commit
to monody0007/edgartools
that referenced
this pull request
Sep 28, 2026
Resolve the conflict with dgunning#1367 in Ownership.shares_traded: keep its None/empty guard and `trades` local, and keep this branch's pd.api.types.is_numeric_dtype check in place of np.issubdtype. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1359
The defect
NonDerivativeTable.market_tradeshas two "nothing to count" states:Nonewhen the non-derivative table has no transactions at all, and an empty frame when it has transactions but none of them is codedPorS. Every caller guards both - the summary path,common_stock_purchases,common_stock_sales,get_ownership_summary- exceptOwnership.shares_traded, which read.Sharesstraight off it.So a derivative-only Form 4 parsed fine and returned its derivative transaction through
to_dataframe(), then raised on the accessor:Reproduced on a clean checkout of
b022ad29(5.59.1) through the publicfiling.obj()path, with the filing the reporter named: Vertex Pharmaceuticals Form 4 filed 2020-10-19, accession0001209191-20-055264. It is already the fixture behindtests/ownership/test_form4.py::test_form4_with_derivatives.The change
shares_tradedreturns0for both states. Two notes on the contract, since the number is easy to misread:0counts non-derivative open-market trades only. It says nothing about derivative activity, which this accessor has never counted. That is now in the docstring rather than implied.Noneis deliberately untouched. A filing that traded an amount we cannot parse must not be reported as having traded nothing. A test pins that.One behaviour change beyond the crash: an empty frame whose
Sharescolumn is not numeric fell through toNonebefore and now returns0. The award-only case already returned0by summing an empty numeric column, so that answer is unchanged.Tests
tests/issues/regression/test_issue_1359_shares_traded_derivative_only.py, four tests: the reproduction, plus the three controls the issue asked for.0001209191-20-055264, derivative-onlyAttributeError0374WaterForm4.xml, awards only (empty frame)00form4.snow.xml, five market trades200000200000SharesNoneNoneThe reproduction test fails on an unfixed tree and the three controls pass on it, so the file pins the fix rather than the fixture.
It reads a checked-in copy of the Vertex XML (
data/ownership/VertexForm4.derivative-only.xml, 4 KB) and stubs the reporting-owner CIK lookup, the only fetch inOwnership.parse_xml, using the same stub astests/ownership/test_insider_parse_contract.py. Verified offline underpytest -p tests._offline_harness, so it lands in thefastpull-request gate rather than the post-merge network lane and needs no entry inREGRESSION_NETWORK_*.The fixture needed
git add -f:/data/is in.gitignorewhile the fixtures inside it are tracked. Say the word if you would rather it lived elsewhere.Verification
Python 3.14.5, pandas 3.0.6, Windows.
pytest tests/ownershipplus the three ownership regression files: 89 passed, exit 0.edgar/ownership/forms.pyreverted.pytest tests/issues/regression -m fast: 13 failures, and the failure set is byte-identical with and without this change. They are pre-existing in my environment and unrelated (test_issue_1325_import_orderingis Importing edgar.reference on one thread while another thread imports edgar deadlocks (_DeadlockError, ImportError) #1325 itself;test_filing_text_baselineand thedt1f1files want fixtures my checkout does not have).ruff check edgar: output byte-identical toupstream/main, so no new findings. Note the tree is already red under ruff 0.16.9 (1091 findings on an untouched checkout), which looks like drift from whatever CI pins.ruff formatis not enforced, so formatting is left alone.scripts/check_regression_provenance.pyandscripts/check_regression_skips.py: both OK.changelog.d/1359.fixed.md, 402 characters.One thing noticed, not fixed
On pandas 3, a genuinely non-numeric
Sharescolumn is inferred asStringDtype, andnp.issubdtyperaisesTypeError: Cannot interpret '<StringDtype(na_value=nan)>' as a data typerather than falling through toNone. That is a separate defect from this one and out of scope here, so the non-numeric test forcesobjectdtype to assert the documented fall-through without depending on it. Happy to file it if it is not already tracked.🤖 Generated with Claude Code