Skip to content

Remove dead PMC full-text code from PMIDSource - #64

Open
TankLin23 wants to merge 1 commit into
linkml:mainfrom
TankLin23:refactor/remove-dead-pmc-code
Open

TankLin23 wants to merge 1 commit into
linkml:mainfrom
TankLin23:refactor/remove-dead-pmc-code

Conversation

@TankLin23

Copy link
Copy Markdown

Closes #52.

What

Removes four dead methods from PMIDSource (src/linkml_reference_validator/etl/sources/pmid.py):

method lines removed
_fetch_pmc_fulltext 339-363
_get_pmcid 365-403
_fetch_pmc_xml 405-439
_fetch_pmc_html 441-472

Plus the now-unused import requests — its only use in the module was inside the deleted _fetch_pmc_html.

Why they are dead

PMIDSource.fetch stopped retrieving PMC full text when that moved to the provider chain in #48. The methods were left behind, and the design plan deferred exactly this cleanup:

docs/superpowers/plans/2026-06-12-modular-pdf-fulltext-fetching.md:1719
"Leave _fetch_pmc_fulltext, _get_pmcid, _fetch_pmc_xml, and _fetch_pmc_html in place for now (still covered by their unit tests); they are simply no longer called from fetch. (A later cleanup may remove them once the provider fully supersedes them.)"

Evidence they are unreachable

The four methods call only each other — nothing outside the block calls them, and fetch() does not:

$ grep -rn "_fetch_pmc_fulltext\|_get_pmcid\|_fetch_pmc_xml\|_fetch_pmc_html" src/linkml_reference_validator/etl/sources/pmid.py
339:    def _fetch_pmc_fulltext(          # only caller of _get_pmcid / _fetch_pmc_xml / _fetch_pmc_html
351:        pmcid = self._get_pmcid(...)  # inside _fetch_pmc_fulltext
355:        full_text = self._fetch_pmc_xml(...)
359:        full_text = self._fetch_pmc_html(...)
365:    def _get_pmcid(...)
405:    def _fetch_pmc_xml(...)
441:    def _fetch_pmc_html(...)

The only self._* calls reachable from fetch() are _parse_authors, _fetch_pubmed_xml, _parse_abstract, _parse_mesh_terms and _parse_publication_types. Every behaviour in the deleted block now lives in PMCFullTextProvider (etl/fulltext/pmc.py): _resolve_pmcid_get_pmcid, _fetch_pmc_xml_bytes_fetch_pmc_xml, _fetch_pmc_html_fetch_pmc_html.

Test coverage is moved, not dropped

test_get_pmcid_handles_entrez_error was the only test covering the deleted _get_pmcid, so I did not simply remove it. The same error path is now asserted against the live implementation:

  • removed: tests/test_sources.py::TestPMIDSource::test_get_pmcid_handles_entrez_error (covered the dead _get_pmcid)
  • added: tests/test_fulltext_providers.py::TestPMCProvider::test_resolve_pmcid_handles_entrez_error — patches Entrez.elink to raise and asserts PMCFullTextProvider._resolve_pmcid returns None

I intentionally did not port the old test's handle.close.assert_called_once(): PMCFullTextProvider._resolve_pmcid wraps elink and read in a single try, so handle.close() is not reached on read failure. The new test asserts the actual contract (None) rather than the old implementation detail.

Stale references

src/linkml_reference_validator/plugins/REVIEW.md still pointed at the removed methods. Dropped:

  • the doctest example (e.g., _fetch_pmc_fulltext) in "Insufficient Doctests for Complex Functions"
  • the three "Untested lines" bullets naming _fetch_pmc_fulltext / _fetch_pmc_xml / _fetch_pmc_html
  • the trailing Impact: PMC full-text retrieval completely untested line, which is no longer true (PMC retrieval is covered by tests/test_fulltext_providers.py::TestPMCProvider) and no longer matches the remaining bullet list

Happy to trim that last one back if you would rather keep the review doc as an unedited historical record.

Verification

Run on refactor/remove-dead-pmc-code, Python 3.11, uv 0.12.7:

$ uv run pytest -q
656 passed

$ uv run pytest --doctest-modules src -q     # just doctest
190 passed

$ uv run ruff check .                        # just format
All checks passed!

$ uv run mypy src tests                      # just mypy
Success: no issues found in 85 source files

Not done

  • No behavioural change to PMIDSource.fetch or PMCFullTextProvider — deletion only.
  • docs/superpowers/plans/2026-06-12-modular-pdf-fulltext-fetching.md and REVIEW2.md still mention these names in their historical narrative; I left both alone as records of the plan rather than live documentation.
  • Note that test_locate_returns_text_from_xml patches _resolve_pmcid with return_value="999" while passing pmcid="999", so the patch is not actually exercised there. That is pre-existing and out of scope for this PR, but flagging it in case it is worth a follow-up.

PMIDSource.fetch no longer retrieves PMC full text - that moved to the
provider chain in linkml#48 - but the PMC helper methods were left behind in
etl/sources/pmid.py with no live caller. The design plan deferred the
cleanup explicitly:

  docs/superpowers/plans/2026-06-12-modular-pdf-fulltext-fetching.md
  "Leave _fetch_pmc_fulltext, _get_pmcid, _fetch_pmc_xml, and
   _fetch_pmc_html in place for now ... A later cleanup may remove them
   once the provider fully supersedes them."

PMCFullTextProvider now owns all four behaviours, so this removes:

- _fetch_pmc_fulltext, _get_pmcid, _fetch_pmc_xml and _fetch_pmc_html
  (pmid.py lines 339-472)
- the now-unused `import requests`; its only use was inside the deleted
  _fetch_pmc_html

Coverage of PMC-ID resolution is moved rather than dropped.
test_get_pmcid_handles_entrez_error covered the deleted _get_pmcid, so it
is replaced by test_resolve_pmcid_handles_entrez_error, which exercises
the same Entrez elink failure path on the live
PMCFullTextProvider._resolve_pmcid.

Also drops the stale references to the removed methods in
plugins/REVIEW.md.

Closes linkml#52
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.

Remove dead PMC full-text code from sources/pmid.py (superseded by PMCFullTextProvider)

1 participant