Skip to content

fix: add missing self to reindex_subcomponent_taxa#233

Merged
mmore500 merged 2 commits into
jeetsukumaran:mainfrom
steps-re:fix/reindex-subcomponent-taxa-self
Jul 12, 2026
Merged

fix: add missing self to reindex_subcomponent_taxa#233
mmore500 merged 2 commits into
jeetsukumaran:mainfrom
steps-re:fix/reindex-subcomponent-taxa-self

Conversation

@steps-re

Copy link
Copy Markdown
Contributor

TaxonNamespaceAssociated.reindex_subcomponent_taxa is defined without self:

def reindex_subcomponent_taxa():
    raise NotImplementedError()

but it's called as self.reindex_subcomponent_taxa() from reindex_taxa(). Any class that relies on the base stub (instead of overriding it) hits a TypeError the moment reindex_taxa() runs, instead of the intended NotImplementedError.

TreeList is one such class: it doesn't override reindex_subcomponent_taxa... wait, it does, but with the identical bug in treecollectionmodel.py:

def reindex_subcomponent_taxa():
    raise NotImplementedError()

Repro before the fix:

import dendropy
tl = dendropy.TreeList.get(data='(A,(B,C));', schema='newick')
tl.reindex_taxa()
# TypeError: TreeList.reindex_subcomponent_taxa() takes 0 positional arguments but 1 was given

Tree.reindex_taxa() and CharacterMatrix.reindex_taxa() work fine since those subclasses override reindex_subcomponent_taxa(self) correctly with self already. This PR adds the missing self to the two remaining definitions so the call succeeds and, for classes without a real implementation, raises NotImplementedError as designed rather than crashing with a confusing TypeError.

Ran the taxon and tree-collection test suites plus the full unit suite locally: 884 passed, 4 pre-existing failures unrelated to this change (they need the external paup binary, which isn't installed in my environment).

TaxonNamespaceAssociated.reindex_subcomponent_taxa was defined
without self but called as self.reindex_subcomponent_taxa() from
reindex_taxa(), raising TypeError instead of the intended
NotImplementedError on any class relying on the base stub (e.g.
TreeList). TreeList's own override in treecollectionmodel.py had
the same bug. Both now take self.

Signed-off-by: Mike German <mike@stepsventures.com>
@mmore500

Copy link
Copy Markdown
Collaborator

Thanks for the contribution @steps-re ! I can add a regression test based on the minimum working bug you provided, and hopefully get this merged in and release a patch shortly.

@codecov-commenter

codecov-commenter commented Jul 12, 2026

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.70%. Comparing base (75bc2ae) to head (2e27011).
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #233   +/-   ##
=======================================
  Coverage   88.69%   88.70%           
=======================================
  Files         114      114           
  Lines       12844    12853    +9     
=======================================
+ Hits        11392    11401    +9     
  Misses       1452     1452           
Flag Coverage Δ
unittests 88.70% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Tests drafted via @claude and hand-reviewed.
@mmore500
mmore500 merged commit cdfb288 into jeetsukumaran:main Jul 12, 2026
29 checks passed
@mmore500

Copy link
Copy Markdown
Collaborator

Thanks again for the contribution @steps-re , the fix should be deployed as v5.0.10. Let us know any other issues you turn up!

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.

3 participants